Problem/Motivation

On a website with big pipe enabled we encountered an issue where every now and then a big pipe placeholder was not being replaced.

To reproduce the issue I refreshed a page containing a big pipe placeholder multiple times with the browser console open. After some 30-50 refreshes I hit the following:

SyntaxError: JSON.parse: unterminated string at line 1 column 16683 of the JSON data

With stacktrace:

bigPipeProcessPlaceholderReplacement
each
each
bigPipeProcessDocument
bigPipeProcess/timeoutID

Then I checked big_pipe.es6.js and found the comment:

      // If we try to parse the content too early (when the JSON containing Ajax
      // commands is still arriving), textContent will be empty which will cause
      // JSON.parse() to fail. Remove once so that it can be processed again
      // later.
      // @see bigPipeProcessDocument()

I tracked down this addition to #2469431-148: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

I don't believe however that just checking textContent is enough to ensure the output of the big pipe replacement has been received completely.

Proposed resolution

(Description of the proposed solution, the rationale behind it, and workarounds for people who cannot use the patch.)

Remaining tasks

(reviews needed, tests to be written or run, documentation to be written, etc.)

User interface changes

None

API changes

None

Data model changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

casey created an issue. See original summary.

casey’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
5.46 KB
casey’s picture

borisson_’s picture

Issue tags: +Needs tests

I don't understand why #3 isn't tested by the bot. Adding the needs tests tag. But if this only happens after ~30 requests it will be really complicated to write a good test for this.

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -14,37 +14,28 @@
-   * @param {HTMLScriptElement} placeholderReplacement
+   * @param {HTMLScriptElement} placeholderReplacementSignal

Is this just a rename for clarity? Because if it's not needed, I think we should keep it as it was. To make it easier to see the actual changes here.

casey’s picture

#3 is a patch for Drupal 8.3 (so we could apply it to the project in which we encountered the issue).

Wim Leers’s picture

Title: JSON.parse sometimes called on partial big pipe replacements » Browser parsing race condition: JSON.parse sometimes called on partially streamed BigPipe placholder replacements, causes fatal JS error
Status: Needs review » Needs work
Issue tags: +JavaScript

I don't believe however that just checking textContent is enough to ensure the output of the big pipe replacement has been received completely.

This sounds indeed plausible. Probably this was fine for the less complex/elaborate cases we tested, because we didn't have >16 KB of data to arrive via JSON, and you do (since you report that it fails at column 16683).

+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -157,6 +157,13 @@ class BigPipe {
+  const REPLACEMENT_SIGNAL = '<script type="application/vnd.drupal-ajax" data-big-pipe-event="replacement"></script>';

@@ -599,6 +606,7 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
     </script>
 EOF;
+      $output .= static::REPLACEMENT_SIGNAL . "\n";
       $this->sendChunk($output);

I like this!


#4: It doesn't happen every 30 requests for certain. This is a client-side (browser) race condition. We may not be able to write test coverage for it. But we definitely should try. Something like sending 16 KB of data, but sleeping for a second between every 1 KB would hopefully trigger it.


#5: this patch needs to be rolled against 8.5 for it to get committed. Also, no bug fixes happen for 8.3 anymore. They will happen for 8.4.

Wim Leers’s picture

Issue summary: View changes
zuuperman’s picture

I had similar issues with my project. Chrome was ok, but firefox was complaining. Manually applying the patch on the 8.4.4 fixed the issue.

Wim Leers’s picture

@zuuperman Thank you very much for confirming! Any chance you'd be up for writing test coverage as I described in #6? :) I'd promise fast reviews!

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

zuuperman’s picture

Sry, Wim. Did not have time.

But an update. I started noticing issues when a block was not cached yet. Some blocks randomly returned empty data, leading to empty blocks displayed. Removing the patch fixes this, but brings back my firefox problem. So the patch is not completely ok.
The moment blocks are cached, the empty block is gone and big pipe replacement behaves normally.

zuuperman’s picture

I created a patch that works for my project. To be 100% sure, I reverted a couple of times to the original patch. Every time I do, one of the blocks is not displayed. After some debugging I noticed it's related with following code:

  function bigPipeProcessPlaceholderReplacement(index, placeholderReplacementSignal) {
    var placeholderReplacement = placeholderReplacementSignal.previousElementSibling;

    var placeholderId = placeholderReplacement.getAttribute('data-big-pipe-replacement-for-placeholder-with-id');

The block that is going wrong, is always a block that comes after a form that has autocomplete enabled. Viewing with inspector

<div role="status" aria-live="assertive" aria-relevant="additions" class="ui-helper-hidden-accessible"></div>
<script type="application/vnd.drupal-ajax" data-big-pipe-event="replacement"></script>

inspector

So the previousElementSibling is not the replacement data. See my console.log of placeholderReplacement and the found response:

replacements

zuuperman’s picture

Included a patch that works for me, without the use of replacement signal.

Optionally we could tweak the replacement signal version, by searching the previous elements by id. But this leads to dom queries that can be avoided.

drpal’s picture

  1. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
    @@ -29,7 +29,17 @@
    +          // If json is not yet streamed completely. Parsing will fail.
    +          // Remove once so that it can be processed again later.
    

    This should follow our standards for multi-line comments.

  2. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
    @@ -29,7 +29,17 @@
    +          return;
    

    This is a bit confusing. Perhaps some additional documentation that we're going to return early, if JSON parsing fails?