Pass RenderContext around in the Renderer

Looking at this it's a lot more complicated than I outlined in the issue summary because the static render context stack is a final protection against container rebuilds in the middle of rendering etc.

What I think we can probably do is when rendering a placeholder within a Fiber, when the fiber is suspended, check if the render context stack changed, and if it did, then immediately resume the fiber to keep going - that should prevent cross-leakage of render context then. I don't have an implementation yet, but something along these lines.

OK I did some investigation with 📌 Try to replace path alias preloading with lazy generation Active and 📌 Deprecate RendererInterface::render()'s sole $is_root_call parameter Needs work combined, and have a fix.

This is proper bug in core but because we don't actually have any runtime code that suspends fibers during rendering yet, it's not triggered by anything.

This something we should add test coverage for?

@smustgrave once you combine the three patches together we have a lot of implicit test coverage for it (as in core blows up without it).

I haven't got my head around how to add explicit test coverage yet, or indeed why the current fibers test coverage doesn't trigger the bug.

is that really a bug? looks more like task kind

@andypost without both fixes, any async code during rendering results in an exception, because we already protect against the escaping but didn't handle it everywhere when adding initial placeholder Fiber support.

@catch that's why I consider it task as async code was never supposed to be supported

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Sorry to be that guy can the issue summary be filled out please.

@andypost we already support async rendering via 📌 Add Fibers support to Drupal\Core\Render\Renderer Fixed and 📌 Add PHP Fibers support to BigPipe RTBC however because core doesn't have any async code yet this bug wasn't hit.

@smustgrave I've updated the issue summary, this is a hard issue to explain (it was also hard to track down and fix), so feedback on whether it's readable would be useful.

#7 is still the current state of test coverage - it's easy to reproduce with the two blocked issues but I'm still struggling to come up with a self-contained test for it. Might have another poke around though

Was able to come up with a unit test that simulates the assertion failure in ::executeInRenderContext() during big pipe rendering when 📌 Try to replace path alias preloading with lazy generation Active is applied, which was otherwise only visible in a couple of functional tests. Coming up with a minimal render array was not trivial - it needs both nested calls to ::executeInRenderContext() and also multiple Fiber::suspends() to trigger the assertion.

MR looks good to me. I am not too familiar with fibers but the comments do a decent job of explaining why this is needed ... There is currently a test failure in Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformance

Not sure why there's an extra cache get in the performance test yet, but did a file_put_contents() of the performance_data object then diffed it, and this is the extra one (would be nice if it looked this nice in the diff output, there's also a load of hashes in cids that are different for probably no good reason):

                             [21] => ckeditor5.langcodes
                             [22] => ckeditor5.langcodes
                             [23] => ckeditor5.langcodes
                             [24] => ckeditor5.langcodes
+                            [25] => ckeditor5.langcodes

That comes from _ckeditor5_get_langcode_mapping(). Will open a spin-off to move that to a service and keep the mappings in a property etc.

Interesting test failure with somehow the attribute order changing:

Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5MarkupTest::testAttributeEncoding
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'...e-images/image-test.png" width="40" height="20" alt="</em> Kittens & llamas are cute">'
+'...e-images/image-test.png" alt="</em> Kittens & llamas are cute" width="40" height="20">'

core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5MarkupTest.php:110

#18 might not be right, debugging I can't see an extra cache get - just that there's too many. So possibly that call just moved around. Will need to look more.

ckeditor5 was a red herring, although that's definitely a small optimisation we can make in another issue.

The extra cache get is from this in Drupal\Core\Theme\Registry

    // If called from inside a Fiber, suspend it, this may allow another code
    // path to begin an asynchronous operation before we do the CPU-intensive
    // task of building the theme registry.
    if (\Fiber::getCurrent() !== NULL) {
      \Fiber::suspend();
      // When the Fiber is resumed, check the cache again since it may have been
      // built in the meantime, either in this process or via a different
      // request altogether.
      if ($cached = $this->cacheGet()) {
        return $cached;
      }
    }

Because the execution is now inside a fiber, this suspend happens, then when it's resumed the cache is requested again.

This was added in 📌 Add PHP Fibers support to BigPipe RTBC .

I think there's a question of how useful this is without 📌 Use revolt to prewarm caches during lock waits Active or async database queries etc., but this explains the extra cache get. For me that means there's nothing to do in this issue although we might want to revisit that elsewhere.

To get a useful diff, I had to remove the hash salt and private key from the user permissions hash - I think we might also want to open a spin-off for that because not only are the permissions hashes never really exposed anywhere, but also there's no useful information in a hash as such anyway - this would allow us to compare cache operations much easier when they're part of cache contexts.

Because the execution is now inside a fiber, this suspend happens, then when it's resumed the cache is requested again.

Nice find.

To get a useful diff, I had to remove the hash salt and private key from the user permissions hash - I think we might also want to open a spin-off for that because not only are the permissions hashes never really exposed anywhere, but also there's no useful information in a hash as such anyway - this would allow us to compare cache operations much easier when they're part of cache contexts.

That comes from _ckeditor5_get_langcode_mapping(). Will open a spin-off to move that to a service and keep the mappings in a property etc.

Adding "Needs followup" tag as a reminder for these two.

I think only things left outstanding are the test failures in core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php and core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5MarkupTest.php. (The SettingsTrayBlockFormTest seems like a recurring intermittent failure.)

Comments are much clear now.

I have two real questions.

Opened 📌 Refactor _ckeditor5_get_langcode_mapping() Active for the ckeditor5 issue.

Revived 📌 Implement xxHash for non-cryptographic use-cases Needs work with a new MR and added the permissions hash generator change there - that issue might need splitting but at least it documents the thought.

OpenTelemetryNodePagePerformanceTest

I think this is a timing issue with the test - it looks like the image style request didn't finish before the performance data started recording. Added a couple of sleep(1) calls.

CKEditor5MarkupTest.php

Re-ran this test and it looks random.