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

Comments

c960657’s picture

c960657 commented
Priority: Normal » Minor
StatusFileSize
new image-lock-1.patch5.83 KB
drewish’s picture

drewish commented

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

c960657 commented
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

drewish commented

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

drewish’s picture

drewish commented
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

dries commented
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

c960657 commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new image-lock-2.patch6.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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
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.