http://api.drupal.org/api/function/image_style_generate/7

    if (!$lock_acquired) {
      // Tell client to retry again in 3 seconds. Currently no browsers are known
      // to support Retry-After.
      drupal_add_http_header('Status', '503 Service Unavailable');
      drupal_add_http_header('Retry-After', 3);
      print t('Image generation in progress. Try again shortly.');
      drupal_exit();

This just looks like it should be a lock_wait(), especially since it apparently relies on behaviour that no browser actually supports.

CommentFileSizeAuthor
#15 image_503_fix-848044-15.patch653 bytesrandallknutson
#9 848044.image_.patch2.82 KBAnonymous (not verified)
#7 image.lock_.wait_.patch4.71 KBAnonymous (not verified)
#5 image.lock_.wait_.patch4.1 KBAnonymous (not verified)
#2 image.lock_.wait_.patch2.77 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

yep, i agree with catch.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: Unassigned »
Status: Active » Needs review
FileSize
image.lock_.wait_.patch2.77 KB

here's a patch for that. code in the patch tries to acquire the lock 10 times, then gives up and sends a "Serve Error" header.

Status: Needs review » Needs work

The last submitted patch, image.lock_.wait_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

ok, that fails because we no longer bail out and send a useless header if we don't get the lock first time.

i could just take that test out, but i want to replace it with something that tests the "another process is rebuilding so we failed to get the lock, but then we got the cache so its all good" scenario.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
image.lock_.wait_.patch4.1 KB

here's a patch without the test that relied on the old behaviour.

its hard to test the lock wait code without async http requests, so i've created this issue: #850782: allow testing lock code via async http calls.

Status: Needs review » Needs work

The last submitted patch, image.lock_.wait_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
image.lock_.wait_.patch4.71 KB

take out the lock check in the test, as we don't create one anymore.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

if ($lock_acquired === TRUE) { is usually too verbose. I didn't look into the logic completely though.

A nice to have enhancement would be http headers like 'Image wait' and 'Image build' like we do for cache misses and builds at #802446: Cache stampede protection: drupal_render() and page cache.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
848044.image_.patch2.82 KB

updated #7 so it applies to HEAD.

Status: Needs review » Needs work

The last submitted patch, 848044.image_.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Assigned: » Unassigned
mikran’s picture

mikran CreditAttribution: mikran commented
Status: Needs work » Needs review

#7: image.lock_.wait_.patch queued for re-testing.

mikran’s picture

mikran CreditAttribution: mikran commented
Status: Needs review » Needs work

Oops, I didn't mean to change anything or actually even re-test the old patch so reverting the status back.

Marco Vervoort’s picture

Marco Vervoort CreditAttribution: Marco Vervoort commented
Issue summary: View changes

I encountered this bug when using the media plugin with CKEditor to insert a new thumbnail of an image into a text. On IE11 this resulted in an 'image not loaded' cross (not always, but more than 50% of the time). The randomness is probably related to timing issues (with the Developer console open the percentage dropped to about 30%).

To be quite honest, it is a bit strange that the first request for a not-yet-existing thumbnail yields a 'HTTP 200' response (albeit after a possibly lengthy delay), and that subsequent requests yield an immediate 'HTTP 503' response (as long as the image is still being generated). It doesn't feel consistent. Does anybody know whether this behaviour was intended, and if so, what the reason was?

In order to solve my immediate problem while changing as little as possible, I've modified image.module so that the code waits 3 seconds for the image to appear, by adding

    $lock_acquired = lock_acquire($lock_name);
+    if (!$lock_acquired) {
+      lock_wait($lock_name, 3);
+      $lock_acquired = lock_acquire($lock_name);
+   }
    if (!$lock_acquired) {

This feelds like it shouldn't have negative side-effects. But an official patch to resolve this issue more thoroughly would be appreciated.

randallknutson’s picture

randallknutson CreditAttribution: randallknutson commented
FileSize
image_503_fix-848044-15.patch653 bytes

Ran into this as well with the media module and ckeditor. It has something to do with IE seeing the image twice and requesting it too quickly. The easiest patch is to fix this response with the code from #14. I made a patch so it can be applied in a make file.