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.
In the 'happened to stumble across it while reading code' category:
The leading comment to the LockBackendInterface definition mentions menu_router_rebuild() but that does not exist anymore, so should be replaced. Points:
- I'm not sure if the comment saying "delete the current data in the database" still applies. Probably not. (But I don't understand the code in a Dumper class that 'dumps' the routes - or rather, I don't see which code stores that data, where.)
- Searched the D8 source for the usage of 'acquire()' to see if there was a better example. I don't think there is, and the code in RouteBuilder::rebuild() is actually nicely readable. I'm not sure if it is an example of a real race condition, though - or whether it's now only a matter of "we shouldn't try to run this code in two parallel threads because that's just wasting resources".
Comment | File | Size | Author |
---|---|---|---|
#5 | 2624256-5-lockbackendinterface-comment.patch | 1.28 KB | roderik |
#3 | 2624256-3-lockbackendinterface-comment.patch | 1.29 KB | roderik |
Comments
Comment #2
dawehnerIn fact that example at the moment is no longer true, because we don't rebuild the routing information on access, but just always on write. Another good example could be the container being compiled and written to cache.
Comment #3
roderikGood point. But I'm not seeing any use of the LockBackendInterface in the container compiling code. That's around \Drupal\Core\DrupalKernel::initializeContainer(), right?
Looking through the uses of acquire in core, I'm not sure of all of them but
As far as I'm seeing this leaves only the obvious usage in Cron::run(). Which is not a comcrete example. So I included a cron invocation as an example. For your review/opinion.
Comment #4
jhodgdonThat looks fairly reasonable to me.... I am not sure that we need to be so specific about aggregator_cron() though, and I had to read the paragraph a few times before I figured out how it all fit together (not a sign of wonderfully perfect documentation, although I did eventually figure it out).
Maybe it would be clearer if we just said something like:
For instance, some implementations of hook_cron() implicitly assume they are running only once, rather than having multiple calls in parallel. To prevent problems with such code, the cron system uses a locking process to ensure that cron is not started again if it is already running.
Comment #5
roderikYou're right, that's much better. More readable, doesn't twist your mind into trying to figure out the specific example... and specifics aren't necessary really.
Patch contains your suggestion as is. That's an implicit RTBC :)
Comment #6
jhodgdonLooks good, thanks! I will take the liberty of RTBC-ing the patch, and you RTBC-d the wording, so that is probably enough reviewing. :)
Comment #7
jhodgdonChanging this back to 8.0 because this is docs and should be changed in both 8.0 and 8.1.
Comment #8
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!