Large placeholders are not processed

Patch with #1 solution.

Patch with #2 solution

Patch with Functional test that does not work.

Hi @dinazaur, you did a really nice job explaining the issue! :)

One more idea though - in approach 1 I don't like an idea of processing chunks out of order; probably we could also check if there are previos unprocessed chunks each time a _new_ chunk is loaded?

Thank you for reporting.

Can we get a test only patch showing the issue.

Please read the summary.

I tried to implement a test for this bug, but could not reproduce it inside headless Chromium, probably the problem is probably that it does not work in the same way as not headless browser. I'll attach a patch with the test if someone wants to play with it, but I'm not sure if that's possible to reproduce it inside Functional tests.

From what I understand it's impossible to implement a test for this.

Could tests not be added like BigPipeStrategyTest

I have attached the patch with the "working" test in #4, the problem with this test is it always passes, but it should not. That's why I also added a link to simplytest site where I simulated same situation, and everyone can check it and see that big pipe does not work. I hope it helps you to understand the problem because this is a duplicate of all the information in the summary.

Ah okay I’ll leave for another reviewer then to see what they think. Don’t want to mark anything without test coverage personally.

The PR contains solution #2 because it seems to be the appropriate way of fixing the issue.

Okay, I actually managed to consistently reproduce the bug on my local machine, hopefully, it will fail now inside Drupal CI.

Fixed cspell error.

The last submitted patch, 18: 3387039-big-pipe-large-content-test-18.patch, failed testing. View results →

Yep, as you can see #18 test failed. In that patch I have attached barebones test without the fix. Instead MR with test and fix green.

To save space what do you think of doing

<div id="big-pipe-large-content">
  {% for i in 0..1000000 %}
    boing
  {% endfor %}
</div>

In the template.

Also hiding the test-only patch so the review bot doesn't pick it up.

Nice catch! Used loop instead of raw text. Just in case I'm attaching a patch with the updated version here, I want to be sure that it will fail.

The last submitted patch, 23: 3387039-big-pipe-large-content-test-23.patch, failed testing. View results →

Nice, it failed.

+1 RTBC

I can confirm the issue since Drupal 10.0. I'm using an XYBlockBase class inheriting BlockBase in my module, that lazy-loads the block body using the big-pipe technology. This class is extensively used in my module, mainly to load lists of rendered entities with content sizes from 10k to more than 1M. Since I've updated my Drupal instance to version 10 last week, all these blocks didn't load its content anymore due to the above described issue of an incomplete script-node, when the MutationObserver is triggered.

Side note: The upgrade to Drupal 10 worked on my local development environment for smaller content (<40k), but failed on the stage server. So the streamed size of the script-node content, when the MutationsObserver is triggered, depends on the runtime environment.

The patch #2 solves this issue for all lazy-loading blocks. The solution of patch #2 is clean and not invasive, because all non-JSON-parsable script node processing is just postponed until the document loading is complete (DOMContentLoaded event).

PS: The references to the non-parsable script nodes stay the same, because only the script-node content is updated by the HTTP streaming of large placeholders.

Bumping this to critical - bits of the page just randomly go missing rendering the site potentially unusable, multiple reports in what look like duplicate issues.

+1 on the approach, could use a couple of comments for future us.

Needs work to address @nod_'s feedback.

Added additional comments to the code, ready for review.

Hiding patches to make it easier to show work is in the MR.

Appears the threads and feedback have been addressed.

I would really like to see some manual testing on this issue from the people reporting 🐛 big_pipe sometimes fails to load blocks Active (or with steps to reproduce from that issue), which seems to be more about lots of placeholders rather than large placeholders but feels like could be a different symptom of the same thing.

If it doesn't fix the issues on there, we might want to consider rolling back 📌 Use Mutation observer for BigPipe replacements Fixed altogether than opening a new issue to re-introduce it along with this fix and anything else necessary.

On the other hand if it does fix people's issues on there, I'd be a lot more confident about committing it.

which seems to be more about lots of placeholders rather than large placeholders

As I said in title: it was a chunk of 300kb with an exception.. In our case that was not a big chunk. When Drupal sends big_pipe chunk using print and flush() it is not sent immediately, it probably depends on the size of the data in the buffer (I have not made investigation of it). So our chunk (just the main menu of site) that was not rendered was sent at the last moment with a bunch of chunks. If it was sent alone it would be rendered for sure. So it may depend on the order and size of chunks -- random.

I'm putting a large placeholder in tests because it's the most reliable way of reproducing this issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/4778, which is the MR version of #2 (with additional comments and tests), fixes the issue in my case, where I was getting a big block not being rendered through BigPipe.

I believe that all the feedback was addressed or explained. The tests come up green for DrupalCI and for GitLab CI.

I think it needs a rebase. Pending that rebase, this would have a +1 or RTBC from me.

Thanks for the manual testing that is great. There is one comment early on in the other issue (#2) that claims the fix from here didn't work for them, but otherwise no-one has confirmed either way except @fjgarlin so let's go ahead and get this in.

I am not qualified to review the js here, but nod_ is and his feedback has been addressed.

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

Impressive discovery, test coverage and fix! 🤩👏

🐛 Media library modal field widget does not render selection checkbox after search Active is another report related to this.

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