Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #251792: Implement a locking framework for long operations, we want to us locks to prevent a theme registry rebuild from being performed by more than one request in parallel
Comment | File | Size | Author |
---|---|---|---|
#35 | 553610-33.patch | 1.3 KB | aleevas |
#15 | interdiff.txt | 784 bytes | jibran |
#12 | drupal-lock_theme_registry_build-553610-12.patch | 790 bytes | InternetDevels |
Comments
Comment #1
dawehnerHere is a first version.
I'm wondering how to test the patch, perhaps someone give me a hint.
Comment #2
pwolanin CreditAttribution: pwolanin commentedthis isn't the part that needs the lock
Comment #3
pwolanin CreditAttribution: pwolanin commentedroughly it should look like this (untested), but we may want to refactor the theme rebuild code a little more.
Comment #4
pwolanin CreditAttribution: pwolanin commentedopps - this doens't have a lock_release().
Also, given that there is more-or-less a single dB operation here (calling cache_set), it's probably not that high a priority to lock it.
Comment #5
catchBump/subscribe. This is a very big cache object to set, and it's a global cache, preventing multiple sets we should definitely consider, even if we don't want to lock the actual registry building to one process.
Comment #6
jibranPatch is so old needs a reroll.
Comment #7
InternetDevels CreditAttribution: InternetDevels commentedReroll
Comment #8
dawehner@InternetDevels
We should add this lock to / around the \Drupal\Core\Theme\Registry::build() method.
Comment #9
InternetDevels CreditAttribution: InternetDevels commentedAnother try
Comment #11
dawehnerWhat about wrapping the call to build() so the patch is way smaller?
Comment #12
InternetDevels CreditAttribution: InternetDevels commentedNew try
Comment #13
dawehner@InternetDevels
This looks much better. Now we just have to inject the lock service instead.
Comment #15
jibranFixed #13.
Comment #17
dawehnerYeah I guess we also have to adapt the unit test here.
Comment #18
star-szrTagging to maybe get attention from people to update the tests. I think this could be done in a later 8.x.x release.
Comment #19
dawehnerI don't see why this cannot land in a patch release, ... it fixes a potential bug.
Comment #20
catchYes agreed. Should be able to happen in a patch release.
Comment #21
aerozeppelin CreditAttribution: aerozeppelin commentedAn attempt to fix failing tests from #15.
Comment #27
markhalliwellComment #29
aleevasThe patch from the comment#21 was rerolled successfully
Comment #32
aleevasAdded fix for last patch.
Comment #34
aleevasAdded a new patch with fixes
Comment #35
aleevasForgot to add patch to the my last comment
Here it is
Comment #36
aleevasComment #37
catchThis can't return false - it needs to return the theme registry. So it would need to attempt to get the theme registry from cache after the lock wait, then either wait again or rebuild anyway if it's still not there.