Problem/Motivation

There are typically two blocks that display local tasks, primary and secondary. They essentially rely on the same static cache and access different data within.

The return value for a miss in the static cache is immediately initialized. If the access checks for those local tasks result in a fiber suspension, then the other blocks is built, which requests the local tasks for the same route and then immediately gets an empty list back.

Steps to reproduce

Enable webform and entity_usage on 11.x, enable entity usage for webforms. Secondary local tasks are somewhat uncommon, this specific combination loads a webform for the usage local task (and because it doesn't load it override free, it doesn't hit the existing static cache)

Either the primary or secondary local tasks will now be missing.

Proposed resolution

Add a fiber suspend if we're in a fiber and already calculating local tasks? Fairly straightforward fix, but I am concerned that we're going to hit more cases like this with static caches.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3553342

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

berdir opened merge request !13539

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Proposed a fix that seems to be fixing this for our case.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Hmm, in this case the fix is simple and you only needed to move an array initialization into an else-block to avoid tripping up the initial isset() check. But not every system will be so easily adjusted to avoid fibers-induced race conditions like these.

However, by moving the array initialization, doesn't that solve the issue we had in the first place? I.e.: Do we still need to suspend the fiber then? I don't think the issue persists if you no longer initialize $this->taskData[$route_name] outside of the route attribute check. Right?

Edit: Suspending the fiber might still have some performance benefits because we don't run the calculations twice.

smustgrave’s picture

smustgrave commented

Possible to add a test case for this?

clayfreeman’s picture

clayfreeman
he/him
Primary language English
commented

Should there be a meta issue to add critical sections for all other static caches? This would seem to be a sensible requirement that should block progress on additional concurrency efforts. (I presume out-of-process caches won't have similar concerns.)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I managed to write a unit test that simulates a fiber suspend in a callback. The test fails hard and not quite like real-world use cases do because I added an explicit assert on the fiber state. Alternatively I'd need to conditionally resume the fiber to simulate the early return that happens in D10 so that I could then assert the actual response. I think this is clearer and easier to understand.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I think a rogue commit snuck in there?

improve test coverage of hasTranslationChanges(), use loadRevisionUnchanged()...

Should there be a meta issue to add critical sections for all other static caches? This would seem to be a sensible requirement that should block progress on additional concurrency efforts. (I presume out-of-process caches won't have similar concerns.)

Ideally we stop using properties as caches altogether. As demonstrated here, they lead to issues. So maybe a meta issue that tracks all the places where we use properties as caches is warranted.

The test approach seems fine, but given how the issue was resolved by moving the variable initialization around, I wonder what we're testing here. If we're only testing whether the fiber is suspended, maybe we could extend the LocalTaskManager for this test and add a public isLoadingLocalTasks() method.

Then we could drop the result checks altogether and merely verify that when isLoadingLocalTasks() returns TRUE, the next fiber gets suspended? We don't really care about the return value of getLocalTasks() here, do we?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Oops, fixed the extra commit that snuck in.

The test fails first on the explicit asserts on the fiber, but it also explicitly asserts the response value. On HEAD, this would also fail because the return value of the second call would be an empty array.

I could make it more agnostic to the exact fiber behavior, I kind of did that on purpose as I wrote it to assert that the mock does suspend at least once, because if I'm just doing a general loop to resume the fibers until they're done then it might also pass if for some reason the fiber was never suspended. But I just had an idea to do the loop but also ensure that it was suspended at least once.

The test fail is clearer then:

1) Drupal\Tests\Core\Menu\LocalTaskManagerTest::testGetTasksBuildWithFibers
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'menu_local_task_test_tasks_view_child1'
-    1 => 'menu_local_task_test_tasks_view_child2'
 )

/var/www/html/core/tests/Drupal/Tests/Core/Menu/LocalTaskManagerTest.php:526

So the result of the second fiber/level 1 is now empty without the fix. This doesn't explicitly test that we suspend to avoid calculating twice, but that's the optimization on top of the fix. The most important thing is IMHO the correct response.

On properties vs cache, I think that would only indirectly help in that using a cache would make it less likely to initialize to an empty result too early, as is the original problem here. We wouldn't think of the extra optimization either, but I still have that idea of a cache wrapper/helper that allows to use that neat Symfony DX of a cache get with a compute callback and optional locking (per preprocess for a memory bin or global) to standardize this. One day I'll manage to flesh that out and create an issue.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

The most important thing is IMHO the correct response.

Eh, good enough for me.

On properties vs cache, I think that would only indirectly help in that using a cache would make it less likely to initialize to an empty result too early, as is the original problem here.

My point exactly.

We wouldn't think of the extra optimization either, but I still have that idea of a cache wrapper/helper that allows to use that neat Symfony DX of a cache get with a compute callback and optional locking (per preprocess for a memory bin or global) to standardize this. One day I'll manage to flesh that out and create an issue.

Yes, ideally we hit two birds with one stone by combining the effort of replacing property caches with the approach you suggested.

catch’s picture

catch
he/him
Primary language English
commented

We could add a sibling issue for #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole and #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() for the class property caches, we have quite a lot of ::reset() methods that can eventually be removed if we clean all of that up. Would also be a follow-up from #3047289: Standardize how we implement in-memory caches.

I think this solution looks good but need to look at it one more time when I have a bit longer in one go.

catch’s picture

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

Moving to needs work for the expanded comment.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Updated the comment.

catch’s picture

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

That looks great. This is something that will eventually need updating once we move to revolt, so good to have the background in there.

  • catch committed 78f170a9 on 11.3.x 
    fix: #3553342 Race condition in LocalTaskManager::getLocalTasks() with...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Tagging needs follow-up for #10 / #11.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed 8adf8b75 on 11.x 
    fix: #3553342 Race condition in LocalTaskManager::getLocalTasks() with...

catch closed merge request !13539

greatmatter’s picture

greatmatter commented

Do we know when this will make it into a release?

nicxvan’s picture

nicxvan commented

It will be in 11.3.2

Status: Fixed » Closed (fixed)

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