Follow-up to #251792: Implement a locking framework for long operations, we want to use locks to prevent concurrent image generation rather than relying on Drupal's cache which are potentially prone to race conditions due to caching.

CommentFileSizeAuthor
#7 image-lock-2.patch6.17 KBc960657
#1 image-lock-1.patch5.83 KBc960657
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

Priority: Normal » Minor
FileSize
5.83 KB
drewish’s picture

I'm not sure if we really get the win here because we're still using cache_image for granting access to temp images.

c960657’s picture

Status: Active » Needs review

What do you mean by “win”? cache_image is used for two unrelated purposes. One of them is a kind of semaphore, but one that doesn't address race conditions like the locking framework does.

drewish’s picture

the ironic thing is that it's used for two unrelated tasks—neither of which is caching...

drewish’s picture

Status: Needs review » Reviewed & tested by the community

ignore my attempt to derail this. it's rtbc. we can sort out the right way for image to do it's permissions later.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Let's write + if (!file_exists($destination) && !($lock_acquired = lock_acquire($lock_name))) { in two lines. Set RTBC after. Thanks!

c960657’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.17 KB

I made it into two if statements to make it even clearer that = is an assignment, not a comparison, and that lock_acquire() is only called when file_exists() returns false.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks good! Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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