Notice: Trying to get property of non-object in image_style_deliver()

+    while (empty($image) && $counter < 10) {
+      $image = image_load($derivative_uri);
+    }

The counter is never incremented.

Also would make more sense as we always want to run the code at least once?

do {
  $image = image_load($derivative_uri);
  $counter++;
} while(empty($image) && $counter < 10);

Also

+      $uri = $image->source;
+      $headers = array(
+        'Content-Type' => $image->info['mime_type'],
+        'Content-Length' => $image->info['file_size'],
+      );

This assignment are pointless why not just

   if (!empty($image)) {
     file_transfer($image->source, array('Content-Type' => $image->info['mime_type'], 'Content-Length' => $image->info['file_size']));
   }

Actually why not something like this

+  $image = FALSE;
   if ($success) {
     if (!empty($lock_acquired)) {
       lock_release($lock_name);
     }

-    $image = image_load($derivative_uri);
-    file_transfer($image->source, array('Content-Type' => $image->info['mime_type'], 'Content-Length' => $image->info['file_size']));
+    $counter = 0;
+    do {
+      $image = image_load($derivative_uri);
+      if (!empty($image)) {
+        file_transfer($image->source, array('Content-Type' => $image->info['mime_type'], 'Content-Length' => $image->info['file_size']));
+        break;
+      }
+      else {
+        $counter++;
+      }
+    } while($counter < 10);
   }
-  else {
+  if (empty($image)) {

Re-roll based on the do while approach.

New versions with comments and usleep.

Doh - take 2 :)

#11: 1762772-d8.patch queued for re-testing.

Hello,

do you have a valid patch for Drupal 7?

Thanks.

Correct me if I'm wrote but I believe this patch has a race condition.

Shouldn't we be doing the usleeps() waiting for the to file_exist() before we release the lock.

Otherwise the file will not exist, and another request can come through and see that the file does not exist, successfully acquire the lock again and attempt to regenerate the file.

It would also simply the code in this patch as I believe you could just loop over file_exists before lock_release().

Additionally the D7 patch no longer applies due to the ?itok changes in latest 2 releases.

Feedback is required, as I believe this lock business could be bringing down my server after I installed imagecache_token's and started seeing a lot of missing image style requests via watchdog. Server was brought down yesterday with out this patch, looking in to it is how I found this issue and patch.

This seems very similar to an issue I had on a D6 project in rebuild_menu with poorly written lock code, although this doesn't seem as bad as we check for the return value of lock_acquire()
#246653: Duplicate entry in menu_router: Different symptoms, same issue comment #228

Here's some code to review to further understand my thinking

Code is for D7.

The case I'm suggesting will cause multiple calls to image_style_create_derivative() to occur when only one is required.

This is the condition I'm talking about.

Because right now we have an issue where image_style_create_derivative() is finished by first thread, file does not exist yet, but lock is released. This allows the second thread to go through, file potentially won't exist and image_style_create_derivative() is called again.

This will overwrite the previous file (i believe) and create potential havoc no?

It looks like you have a good grip on this issue. It's probably best that you create the patches to resolve it.

file_unmanaged_move() copy & unlink patch needs to get into core. Seems like this could be a likely cause of this happening more often than it should.

And yes, the reason I found this issue was because of $file->uri not being set, after I installed the imagecache_token's module.

Here's the issue for imagecache_token's with some hacky fixes.
#1600266: no more uri property returned

After applying my patch, and some hacky fixes to imagecache_token module, I'm not longer seeing the empty($file->uri) issue though.

I can test a D7 patch for you (on a live server with traffic), but not a D8 as i don't have it installed.

Haven't tired the patch in #25 yet but the patch in #11 has been running on our live site since sometime at the end of January and we have had no issues. I will try update the patch tomorrow but it may be a couple of weeks before it actually hits the live server. The site in question has a decent amount of traffic and a good amount of nodes with images being created on a daily basis.

Does it make sense to return anything other than a 500 error? 503 with a retry-after header (choosing a time is impossible to predict). 500 errors for me mean, something went wrong, while this error is most likely "resource is not yet available."

I'll put the patch on my live server today, and check the logs later and let you know how it works.

Discussing changing the error status seems like a different and follow up issue to me, lets keep this focused on fixing the race condition.

Alright, I've made this D7 patch live on my servers. Going to have a wait a couple days to see if I get any error messages. Although since they were removed with only my 1/2 fix in #19 and this patch also includes that, I doubt I will have any problems.

PS. The D7 patch could use a little formatting & logic clean up before it's RTBC. Difference in comment new lines bugs me at least.

I believe my "cleanup" was referring to checking to see if the lock was released succesfully and performing additional logic. In all honesty I have no idea.

With that said, I've had this patch live since may 9th, and have yet to run into this issue again.

RTBC for me.

This was never committed to Drupal 8.

Also not clear this is Drupal's bug to fix, or if it is, that it's being fixed in the right place. When the image is saved, the image toolkit's save function is called, followed by clearstatcache() (see code in image_save()). If the file doesn't reliably exist after that point, is that really Drupal's bug to fix? And if it is (i.e. if we want to work around the multiple webhead issue), shouldn't it be fixed somewhere in that code instead?

Also, if the lock is being released too early, why not just move the lock release code later (rather than having two independent do-while loops)? There should really only be a need for a single do-while loop.

@mikeytown2,

Here's the generic Image::save() — called in $image->save($derivative_uri).

This one calls the toolkit specific save. By default GDToolkit::save() is called.