http://api.drupal.org/api/function/drupal_build_css_cache/6
http://api.drupal.org/api/function/drupal_build_js_cache/6
http://api.drupal.org/api/function/_color_save_stylesheet/6

All 3 of these function call file_save_data() with FILE_EXISTS_REPLACE. file_move() -> file_save_data() -> file_move() -> file_copy(). Once in file_copy it finally calls copy(). The copy function is slow in comparison to rename(); rename is also safer in this case, less likely for a race condition.

Files: 
CommentFileSizeAuthor
#52 drupal-818818-52-file-save-D8.patch1.75 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 50,060 pass(es).
[ View ]
#51 drupal-818818-51-file-save-D7.patch1.73 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 39,577 pass(es).
[ View ]
#50 drupal-818818-50-file-save-D6.patch2.2 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#47 drupal-818818-47-file-save-D8.patch1.75 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 40,397 pass(es).
[ View ]
#47 drupal-818818-47-file-save-D7-do-not-test.patch1.73 KBmikeytown2
#47 drupal-818818-47-file-save-D6-do-not-test.patch2.22 KBmikeytown2
#46 file-save-818818-46.patch1.75 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 34,451 pass(es).
[ View ]
#45 copy_remane_benchmarks .txt1.28 KBoriol_e9g
#41 drupal-818818-41-D7.patch1.92 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-41-D7.patch.
[ View ]
#37 drupal-818818-37-D7.patch1.76 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 22,024 pass(es).
[ View ]
#36 drupal-818818-H-D7.patch1.64 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,927 pass(es).
[ View ]
#34 drupal-818818-G-D7.patch1.64 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,889 pass(es), 34 fail(s), and 135 exception(es).
[ View ]
#32 drupal-818818-F-D7.patch1.65 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,886 pass(es), 34 fail(s), and 135 exception(es).
[ View ]
#31 drupal-818818-A-D7.patch1.62 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,890 pass(es), 34 fail(s), and 131 exception(es).
[ View ]
#31 drupal-818818-B-D7.patch1.62 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,900 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
#31 drupal-818818-C-D7.patch1.62 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,897 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
#31 drupal-818818-D-D7.patch1.61 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,930 pass(es).
[ View ]
#31 drupal-818818-E-D7.patch1.61 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,931 pass(es).
[ View ]
#30 drupal-818818-D6.patch1.63 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-D6_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 drupal-818818-A-D7.patch1.6 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,886 pass(es), 34 fail(s), and 145 exception(es).
[ View ]
#30 drupal-818818-B-D7.patch1.61 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,892 pass(es), 34 fail(s), and 135 exception(es).
[ View ]
#30 drupal-818818-C-D7.patch1.62 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,894 pass(es), 34 fail(s), and 131 exception(es).
[ View ]
#30 drupal-818818-D-D7.patch1.6 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,893 pass(es), 34 fail(s), and 135 exception(es).
[ View ]
#30 drupal-818818-E-D7.patch1.59 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,884 pass(es), 34 fail(s), and 141 exception(es).
[ View ]
#24 drupal-818818-D6.patch1.07 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-D6_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 drupal-818818-D7.patch1.17 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,927 pass(es).
[ View ]
#21 drupal-818818-D7.patch1.19 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,889 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
#20 drupal-818818-D7.patch1.17 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,931 pass(es).
[ View ]
#19 drupal-818818-D7.patch1.17 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,928 pass(es).
[ View ]
#18 drupal-818818-D7.patch1.4 KBmikeytown2
PASSED: [[SimpleTest]]: [MySQL] 20,921 pass(es).
[ View ]
#15 drupal-818818-D7.patch1.39 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,849 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
#11 drupal-818818-D7.patch1.2 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,851 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
#9 drupal-818818-D7.patch1.18 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,863 pass(es), 30 fail(s), and 131 exception(es).
[ View ]
#6 drupal-818818-D7.patch1.12 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,856 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
#4 drupal-818818-D7.patch1.08 KBmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in file.inc.
[ View ]
#3 drupal-818818-D7.patch985 bytesmikeytown2
FAILED: [[SimpleTest]]: [MySQL] 20,849 pass(es), 34 fail(s), and 185 exception(es).
[ View ]
#1 drupal-818818-D6.patch873 bytesmikeytown2
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new873 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-D6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is similar to what I use in the boost module.

Version:6.x-dev» 7.x-dev
Status:Needs review» Needs work

file_copy() should not remove the source file. We need to introduce a temporary file, possibly in the same directory (because renames across partitions/mounts are not atomic).

This needs to be fixed in HEAD and then backported.

Status:Needs work» Needs review
StatusFileSize
new985 bytes
FAILED: [[SimpleTest]]: [MySQL] 20,849 pass(es), 34 fail(s), and 185 exception(es).
[ View ]

7.x equivalent is file_unmanaged_copy() from what I can tell.

StatusFileSize
new1.08 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in file.inc.
[ View ]

Patch with rename in the same dir level now & early abort if the first copy fails.

Status:Needs review» Needs work

The last submitted patch, drupal-818818-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.12 KB
FAILED: [[SimpleTest]]: [MySQL] 20,856 pass(es), 28 fail(s), and 131 exception(es).
[ View ]

Think I fixed the php error & incorrect calling of drupal_tempnam.

subscribing

Status:Needs review» Needs work

The last submitted patch, drupal-818818-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
FAILED: [[SimpleTest]]: [MySQL] 20,863 pass(es), 30 fail(s), and 131 exception(es).
[ View ]

trying file_put_contents after the drupal_tempnam call instead of copy.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
FAILED: [[SimpleTest]]: [MySQL] 20,851 pass(es), 28 fail(s), and 131 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-818818-D7.patch, failed testing.

Looks like I need to fire up 7.x in order to figure this one out.

When I run the test locally I get errors on file_unmanaged_delete_recursive & file_unmanaged_delete.

unlink(C:\xampplite\htdocs\d7\sites\default\files\simpletest\312646\private\temp\.htaccess): Permission denied - file_unmanaged_delete()
rmdir(C:\xampplite\htdocs\d7\sites\default\files\simpletest\312646\private\temp): Directory not empty - file_unmanaged_delete_recursive()

These are the only errors I get. Any ideas?
.htaccess is marked read only, is this the issue?

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
FAILED: [[SimpleTest]]: [MySQL] 20,849 pass(es), 28 fail(s), and 131 exception(es).
[ View ]

This patch passes all tests on my local box.
Why did I have to add in chmod($path, 0777) to file_unmanaged_delete()?

Edit:Think I ran the wrong test (File), should have been (File API). Have a feeling this will bomb as well... I'll see how the local one goes for me...

Status:Needs review» Needs work

The last submitted patch, drupal-818818-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.4 KB
PASSED: [[SimpleTest]]: [MySQL] 20,921 pass(es).
[ View ]

Going to try it without using drupal_tempnam. I have a feeling this function isn't working as I'm expecting it to.

StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 20,928 pass(es).
[ View ]

without chmod

StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 20,931 pass(es).
[ View ]

Trying $temp_name = drupal_tempnam(dirname($destination . '/'), 'file');

StatusFileSize
new1.19 KB
FAILED: [[SimpleTest]]: [MySQL] 20,889 pass(es), 28 fail(s), and 131 exception(es).
[ View ]

Helps to grab the right file...

Status:Needs review» Needs work

The last submitted patch, drupal-818818-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [MySQL] 20,927 pass(es).
[ View ]

Going ahead & not using drupal_tempnam. Besides this point, how does the patch look?

StatusFileSize
new1.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-D6_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll of 6.x based off the latest changes to the 7.x patch

Status:Needs review» Needs work

+  if ($replace == FILE_EXISTS_REPLACE) {
Please add a comment explaining why we have two different code paths here.

The else part covering the two other modes could be extended with a fopen($file, 'x') call to ensure that existing files are never overwritten (another race condition). But that is probably a separate issue.

You may also add a comment mentioning that the unlink() call is necessary, because rename() cannot overwrite existing files on Windows. It would be nice if we could avoid the unlink() on platforms where rename() supports overwriting, though (one approach can be seen here: http://www.smarty.net/forums/viewtopic.php?t=6956&postdays=0&postorder=a...).

+    $temp_name = $destination . '-' . mt_rand();
+    if ($temp_name && @copy($source, $temp_name)) {
$temp_name will always evalute to TRUE here.

Could we choose a name that looks more "temporary"? E.g. add ".tmp" or something.
Elsewhere in core, e.g. in lock.inc we use uniqid(mt_rand(), TRUE) for generating a unique random identifier. But why not use drupal_tempnam() ? If there is a good reason, please add a comment explaining why.

@c960657
I tried drupal_tempnam many times in different ways but it always fails the testbot. That's the reason there are so many fails in this thread.

Why does the testbot not like it?

Do the tests pass on your local machine? If they do, we should fix the testbot (or whatever is causing the tests to fail), rather than implementing workarounds.

file_directory_temp uses OS detection so I wouldn't feel bad doing it again in core

Option A

<?php
     
if ($temp_name && @copy($source, $temp_name)) {
        if (!
$result = @rename($temp_name, $dest)) {
          @
unlink($dest);
         
$result = @rename($temp_name, $dest);
        }
      }
?>

Option B

<?php
     
if ($temp_name && @copy($source, $temp_name)) {
        if (
substr(PHP_OS, 0, 3) == 'WIN' && file_exists($dest)) {
          @
unlink($dest);
         
$result = @rename($temp_name, $dest);
        }
        else {
         
$result = @rename($temp_name, $dest);
        }
      }
?>

tests fail on my local box

Status:Needs work» Needs review
StatusFileSize
new1.59 KB
FAILED: [[SimpleTest]]: [MySQL] 20,884 pass(es), 34 fail(s), and 141 exception(es).
[ View ]
new1.6 KB
FAILED: [[SimpleTest]]: [MySQL] 20,893 pass(es), 34 fail(s), and 135 exception(es).
[ View ]
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 20,894 pass(es), 34 fail(s), and 131 exception(es).
[ View ]
new1.61 KB
FAILED: [[SimpleTest]]: [MySQL] 20,892 pass(es), 34 fail(s), and 135 exception(es).
[ View ]
new1.6 KB
FAILED: [[SimpleTest]]: [MySQL] 20,886 pass(es), 34 fail(s), and 145 exception(es).
[ View ]
new1.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-D6_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

added comments & went for option A
I have 5 different ways of creating the temp file; 2 without drupal_tempnam (A & E) 3 with (B, C & D).

StatusFileSize
new1.61 KB
PASSED: [[SimpleTest]]: [MySQL] 20,931 pass(es).
[ View ]
new1.61 KB
PASSED: [[SimpleTest]]: [MySQL] 20,930 pass(es).
[ View ]
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 20,897 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 20,900 pass(es), 28 fail(s), and 131 exception(es).
[ View ]
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 20,890 pass(es), 34 fail(s), and 131 exception(es).
[ View ]

... can't get this right. Here are all of the D7 done again.

StatusFileSize
new1.65 KB
FAILED: [[SimpleTest]]: [MySQL] 20,886 pass(es), 34 fail(s), and 135 exception(es).
[ View ]

Found 2 functions
drupal_dirname
drupal_realpath
I think I can make this work correctly now...

Status:Needs review» Needs work

The last submitted patch, drupal-818818-F-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 20,889 pass(es), 34 fail(s), and 135 exception(es).
[ View ]

not using drupal_dirname

Status:Needs review» Needs work

The last submitted patch, drupal-818818-G-D7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 20,927 pass(es).
[ View ]

not using drupal_realpath

StatusFileSize
new1.76 KB
PASSED: [[SimpleTest]]: [MySQL] 22,024 pass(es).
[ View ]

remove the temp file if rename or copy failed

Priority:Normal» Major

Why not assign result in copy

++    if ($temp_name && $result = @copy($source, $temp_name)) {
+      // Try the rename opperation
+      if (!$result = @rename($temp_name, $destination)) {
+        // Unlink and try again for windows since rename on windows does not
+        // replace the file if it already exists.
+        @unlink($destination);
+        $result = @rename($temp_name, $destination);
+      }
+    }
-+    else {
-+      $result = FALSE;
-+    }

Is there any benchmarks about copy and rename?

Component:base system» file system
Status:Needs review» Needs work

+++ includes/file.inc 2 Jul 2010 22:32:39 -0000
@@ -677,8 +677,38 @@ function file_unmanaged_copy($source, $d
+  // Perform the replace operation. copy() is not atomic but rename() is.

It's not clear to me what "atomic" means in this context. The further description seems to try to explain parts of it, but overall it looks like the problem space that caused this (ugly) workaround is not properly explained.

+++ includes/file.inc 2 Jul 2010 22:32:39 -0000
@@ -677,8 +677,38 @@ function file_unmanaged_copy($source, $d
+  // Since there could be mutiple processes writing to the same file the best
+  // option is to create a temp file in the same directory and then rename it

1) Typo in "mutiple".

2) "temp" should be "temporary". (and elsewhere)

+++ includes/file.inc 2 Jul 2010 22:32:39 -0000
@@ -677,8 +677,38 @@ function file_unmanaged_copy($source, $d
+    // Get temp filename
...
+    // Place new contents in temp file
...
+      // Try the rename opperation
...
+    // Cleanup tempname if rename/copy failed

1) Missing trailing periods.

2) Typo in "opperation".

Powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-818818-41-D7.patch.
[ View ]

@sun
Thanks for the review. Latest changes to the code. Hopefully the comments explain the code better, and why it's needed.

@andypost
We don't care if the copy to the temporary file worked; we care if the copy to the destination works. That is why I assigned $result = @rename($temporary_file, $destination) and not do it here @copy($source, $temporary_file)

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Bumping and tagging per standard policy.

Issue tags:-needs backport to D7

#41: drupal-818818-41-D7.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, drupal-818818-41-D7.patch, failed testing.

StatusFileSize
new1.28 KB

@andypost: Just out of curiosity: copy() / rename() benchmarks in my localhost: Seems that a single copy operation is slower than a rename operation, but a lot of consecutively copy operations are faster O.o

Benchmarks copy() & rename() functions:
-------------------------------------------------------------------------
1 copy operations in 0.0011789798736572 ms.
1 rename operations in 0.00067996978759766 ms.
rename() is 173.38709677419 times faster than copy().
-------------------------------------------------------------------------
10 copy operations in 0.0082869529724121 ms.
10 rename operations in 0.0071439743041992 ms.
rename() is 115.99919903885 times faster than copy().
-------------------------------------------------------------------------
100 copy operations in 0.052769184112549 ms.
100 rename operations in 0.068475961685181 ms.
copy() is 129.76505670266 times faster than rename().
-------------------------------------------------------------------------
1000 copy operations in 0.32604908943176 ms.
1000 rename operations in 1.1666119098663 ms.
copy() is 357.80253577751 times faster than rename().

Code used for benchmarking attached.

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 34,451 pass(es).
[ View ]

Ported to D8, NR for testbot.

Status:Needs work» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new2.22 KB
new1.73 KB
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 40,397 pass(es).
[ View ]

#46 rerolled and back ported to D7 & D6

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, drupal-818818-47-file-save-D8.patch, failed testing.

#47: drupal-818818-47-file-save-D8.patch queued for re-testing.

Got a fatal error on line #37... looks like testbot messed up, so going to retest
Screenshot of testpage: http://drupal.org/node/1158322#comment-6417676

EDIT: Test looks good now :)

Version:8.x-dev» 6.x-dev
StatusFileSize
new2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Want to make sure patch passes testbot. Fixed 2 errors that I found when back porting the patch to 6.x as drupal_tempnam and drupal_dirname are 7.x+ functions.

Version:6.x-dev» 7.x-dev
StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 39,577 pass(es).
[ View ]

Testing the 7.x patch now.

Version:7.x-dev» 8.x-dev
Issue tags:-needs backport to D7
StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 50,060 pass(es).
[ View ]

Moving back to 8.x and waiting...

#52: drupal-818818-52-file-save-D8.patch queued for re-testing.