Problem/Motivation

file_unmanaged_save_data() uses cp, which is not atomic.

Only link() or rename() are atomic.

Also in a scenario where there are multiple webservers, and one is updated before another, there can be a race condition on deployments - see comments in the patch for full explanation.

Proposed resolution

Add a helper, write to temporary file, rename().

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 2628918.patch9.08 KBcatch
#7 2628918.patch10.48 KBcatch
#6 2628918.patch8.2 KBcatch
#5 2628918.patch8.23 KBcatch
#4 2628918.patch8.23 KBcatch
#3 2628918.patch8.93 KBcatch
#2 2628918.patch2.37 KBcatch

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2628918.patch2.37 KB

Patch.

catch’s picture

catch
he/him
Primary language English
commented
Title: Workaround lack of atomicity in file_unmanaged_save_data() » Address deployment race conditions
Issue summary: View changes
StatusFileSize
new 2628918.patch8.93 KB

Widening scope of this, since patch touch exactly the same code.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2628918.patch8.23 KB
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2628918.patch8.23 KB
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2628918.patch8.2 KB
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2628918.patch10.48 KB

Also removing the newly added database table, all we need is a cache entry.

The new table means fatal errors before update runs when the module updates, which is unnecessary breakage.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 2628918.patch9.08 KB
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

Committed/pushed to 7.x-1.x.

  • catch committed 098c684 on 7.x-1.x 
    Issue #2628918 by catch: Address deployment race conditions

  • catch committed 2e04593 on 7.x-1.x 
    Issue #2628918 by catch: (follow-up) Address deployment race conditions
catch’s picture

catch
he/him
Primary language English
commented

Just committed/pushed this follow-up: drupal_random_bytes() sometimes returns null bytes which is_dir() and friends don't like.

diff --git a/agrcache.module b/agrcache.module
index 3de10d2..51276f1 100644
--- a/agrcache.module
+++ b/agrcache.module
@@ -471,7 +471,9 @@ function agrcache_generate_aggregate($filename, $type) {
 function _agrcache_file_unmanaged_save_data($data, $path, $uri) {
   // Create the file.
   file_prepare_directory($path, FILE_CREATE_DIRECTORY);
-  $tmp_uri = $uri . drupal_random_bytes(10);
+  // PHP file functions do not like null bytes, so as well as randomizing the
+  // temporary filename, hash it too.
+  $tmp_uri = drupal_hash_base64($uri . drupal_random_bytes(10));
   // file_unmanaged_save_data() uses copy, which is not atomic and could
   // result in empty files being served from other requests while the file
   // is being written. So write it to a temporary filename, then use

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.