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".
CommentFileSizeAuthor
#5 2624256-5-lockbackendinterface-comment.patch1.28 KBroderik
#3 2624256-3-lockbackendinterface-comment.patch1.29 KBroderik
lockbackendinterface-comment.patch1.32 KBroderik

Comments

roderik created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Lock/LockBackendInterface.php
@@ -13,12 +13,11 @@
+ * processes) will execute in parallel. This leads to potential conflicts,
+ * race conditions or unnecessary resource usage, when two requests execute the
+ * same code at the same time. A common example of this is a rebuild like
+ * RouteBuilder::rebuild() where we invoke many hook implementations to get and
+ * process data from all active modules.

In 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.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review
StatusFileSize
new 2624256-3-lockbackendinterface-comment.patch1.29 KB

Good 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

  • the theme registry and MenuRouterRebuildSubscriber are probably also "on write" or "on rare event" only, which means they're not suitable;
  • several backend/test classes I'm skipping because - because;
  • the usage in Comment is non-standard

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.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

That 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.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new 2624256-5-lockbackendinterface-comment.patch1.28 KB

You'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 :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks! I will take the liberty of RTBC-ing the patch, and you RTBC-d the wording, so that is probably enough reviewing. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 8.1.x-dev » 8.0.x-dev

Changing this back to 8.0 because this is docs and should be changed in both 8.0 and 8.1.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 91ff7ed on 
    Issue #2624256 by roderik, jhodgdon, dawehner: Fix mention of...

  • catch committed 81d2014 on 
    Issue #2624256 by roderik, jhodgdon, dawehner: Fix mention of...

Status: Fixed » Closed (fixed)

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