Problem/Motivation

  1. We want D8's authenticated user page loads to be fast.
  2. Some parts of the page are cacheable across users. To actually cache that across users, we have #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).
  3. Other parts need to be dynamically calculated per user, or are simply uncacheable.
  4. Those dynamic parts should not prevent us from showing the rest of the page first.

(Drupal 8's anonymous user page loads already are fast since #606840: Enable internal page cache by default.)

Proposed resolution

We take inspiration from Facebook's BigPipe rendering strategy.

BigPipe demo: https://www.drupal.org/project/big_pipe_demo

  1. Drupal renders HTML pages using render arrays.
  2. Render arrays are a tree data structure that abstractly describe the structure (semantical+visual) of a HTML document. They're really render trees, which are a commonly used abstraction for software that needs to render something. Think of them like a browser's DOM+CSSOM, but with a higher level of abstraction.
  3. Render arrays now contain cacheability metadata: https://www.drupal.org/developing/api/8/render/arrays/cacheability.
  4. Thanks to that cacheability metadata, we can automatically infer which parts of the render tree are too dynamic to be cached. We can automatically detect these "islands of dynamicness":
    1. Subtrees that have max-age = 0 are not cacheable at all.
    2. Subtrees that have "high-cardinality cache contexts" (such as the 'user' cache context, which caches per user, or 'route', which caches per route=route name+params) are cacheable, but would cause such enormous amounts of variations, that the cache hit ratios would be very low.

    In either case, we want to replace these "islands of dynamicness" with placeholders, that we render separately, after rendering the ("main") render tree.

    If we wouldn't do that, then these "islands of dynamicness" would "infect" the entire render tree, causing the entire render tree to become uncacheable, or to have so many variations that cache hit ratios would be very low.

    See https://www.drupal.org/developing/api/8/cache/contexts.

  5. Note that because rendering a render array ("the render tree") is a matter of traversing the tree depth-first, we can automatically discover the parts of the render array that are "too dynamic", and replace them with placeholders. That means we're able to not only replace blocks with placeholders, but anything, i.e. any subtree! It could be something tiny!
    For example: it's perfectly possible to render cache an entire block, but let that block contain a placeholder that we need to render after the page has loaded, because only a small part of that block actually is too dynamic to render cache!
  6. To ensure we are aware of all placeholders on a page, we add "placeholders" as another type of "bubbleable metadata" to \Drupal\Core\Render\BubbleableMetadata, next to #post_render_cache and #attached. That way, after rendering the entire render tree, we'll know all placeholders that need to be replaced, just like we know all assets that need to be loaded (thanks to #attached having bubbled up the render tree).
  7. There are many possible methods to replace those placeholders with their final values (markup): Single-Flush (which is what HEAD does), ESI, BigPipe, ESI-in-JS … Drupal needs a selection mechanism to determine which method should be used. Selection mechanism: TBD.
  8. With a method selected, the placeholders will be replaced with method-specific placeholders: ESI needs <esi:include> tags, BigPipe needs something else, and Single-Flush just renders the final markup immediately (because, as the name says, it wants to deliver the document in a single flush, which is HEAD's behavior).

Concrete example

Let's look at a simple example:

PAGE
  |- BLOCK A (max-age = infinite, tags = [x], contexts = ['user.permissions'])
  |- BLOCK B (max-age = infinite, tags = [y], contexts = ['user.permissions'])
  |- BLOCK C (max-age = 0)
  |- BLOCK D (max-age = infinite, tags = [], contexts = ['user'])
  |- BLOCK E (max-age = infinite, tags = [z], contexts = ['user.roles'])

In this example page, blocks A, B and E are cacheable just fine. But blocks C and D aren't: block C is not cacheable at all (needs to be calculated on every page load), block D varies per user. If we'd bubble their cacheability metadata as we normally would, then the entire page would not be cacheable. But the majority of the page is cacheable! So, instead, we want to serve the page with the cacheable blocks A, B and E first, and then render blocks C and D after the page has loaded.
Note that we could cache block D: caching a block per user is probably acceptable, but caching a route's response per permissions, roles and user is definitely not: that'll cause too many variations. We'll just have to load block D after the page has already loaded, so that the output that is cacheable across authenticated users is visible ASAP.

In other words: we don't want to let blocks C or D "infect" the cacheability of the entire page.

Remaining tasks

  1. Step 1: "placeholders:
    1. #2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback
    2. #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache Assigned to: Wim Leers
  2. Step 2: auto-placeholdering: #2499157: [meta] Auto-placeholdering
  3. Step 3: render strategies: abstraction + SingleFlush (=== HEAD):
    1. #2407195: Move attachment processing to services and per-type response subclasses
    2. #2349011: Support placeholder render strategies so contrib can support BigPipe, ESI…
  4. Step 4: BigPipe render strategy (this issue)

Remaning task list:

  1. Add SessionExistsCacheContext to Drupal 8 core; it's useful to more than just BigPipe, and should therefore not live in the BigPipe module: #2671988: Add SessionExistsCacheContext ('session.exists')
  2. Test coverage: #2628744: Test coverage, see #210
  3. Refine README — see https://www.drupal.org/documentation/modules/big_pipe + https://www.drupal.org/documentation/modules/big_pipe/environment, done in #244
  4. Make it easy to set up a demo, and provide a live demo: #2627016: SimplyTest.me demo + live demo
  5. Test coverage/hardening against #212.4, follow-up created: #2678568: Ensure good UX & DX even when A) rendering of placeholder fails, B) some response event subscriber fails.
  6. Reviews

User interface changes

None.

API changes

None.

Original problem/motivation section by Fabianx

blocked by #2469277: Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption (or will be, after the proof of concept phase)

- We want core to be fast
- We want to ensure we can cache pages that have blocks with max-age=0 (set directly from the config) [IMPLEMENTED]
- We want to ensure we can cache pages that have blocks with a bubbled max-age=0 [@todo]
- We can optionally stream data via JS after the fact

Follow-ups:

- We want to replace high-cardinality cache contexts (e.g. user) with a placeholder (set directly from config)
- We want to replace high-cardinality cache contexts (e.g. user) that come bubbled up with a placeholder

CommentFileSizeAuthor
#274 Screen Shot 2016-06-01 at 4.21.50 PM.png97.46 KBRavindraSingh
#265 interdiff.txt4.31 KBWim Leers
#264 interdiff.txt938 bytesWim Leers
#264 big_pipe-2469431-263.patch121.41 KBWim Leers
#262 interdiff.txt7.65 KBWim Leers
#262 big_pipe-2469431-262.patch120.15 KBWim Leers
#251 interdiff.txt5.44 KBWim Leers
#251 big_pipe-2469431-250.patch119.36 KBWim Leers
#245 interdiff.txt938 bytesWim Leers
#245 big_pipe-2469431-245.patch119.47 KBWim Leers
#244 interdiff.txt5.4 KBWim Leers
#244 big_pipe-2469431-243.patch119.36 KBWim Leers
#244 create_bigpipe_patch-243.sh_.txt484 bytesWim Leers
#242 interdiff.txt13.37 KBWim Leers
#242 big_pipe-contrib-to-core-242-do-not-test.patch9.26 KBWim Leers
#242 big_pipe-2469431-242.patch124 KBWim Leers
#232 interdiff.txt1.45 KBWim Leers
#232 big_pipe-2469431-232.patch125.12 KBWim Leers
#230 interdiff.txt4.03 KBWim Leers
#230 big_pipe-contrib-to-core-230-do-not-test.patch9.48 KBWim Leers
#230 big_pipe-2469431-230.patch124.9 KBWim Leers
#220 big_pipe-2469431-220.patch119.29 KBswentel
#220 2469431-220-interdiff.txt1.25 KBswentel
#218 interdiff.txt5.78 KBWim Leers
#218 big_pipe-2469431-218.patch119.23 KBWim Leers
#216 interdiff.txt938 bytesWim Leers
#216 big_pipe-2469431-216.patch118.2 KBWim Leers
#214 big_pipe-2469431-214.patch118.09 KBWim Leers
#214 create_bigpipe_patch-214.sh_.txt409 bytesWim Leers
#214 interdiff.txt1.04 KBWim Leers
#214 big_pipe-contrib-to-core-214-do-not-test.patch9.48 KBWim Leers
#210 big_pipe-contrib-to-core-210-do-not-test.patch8.46 KBWim Leers
#210 big_pipe-2469431-210.patch117.66 KBWim Leers
#199 create_bigpipe_patch-199.sh_.txt349 bytesWim Leers
#199 bigpipe-2469431-199-do-not-test.patch65.18 KBWim Leers
#195 bigpipe-2469431-195-do-not-test.patch54.15 KBWim Leers
#191 bigpipe-2469431-191-do-not-test.patch54.81 KBWim Leers
#1 implement_bigpipe-2469431-1.txt27.17 KBFabianx
#2 implement_bigpipe-2469431-1.patch27.17 KBFabianx
#3 interdiff.txt6.92 KBFabianx
#3 implement_bigpipe-2469431-3.patch33.03 KBFabianx
#5 interdiff.txt4.08 KBFabianx
#5 interdiff--2.txt7.74 KBFabianx
#5 implement_bigpipe-2469431-4.patch33.08 KBFabianx
#9 interdiff.txt5.96 KBFabianx
#9 implement_bigpipe-2469431-9.patch35.9 KBFabianx
#10 interdiff.txt5.14 KBFabianx
#10 implement_bigpipe-2469431-10.patch39.47 KBFabianx
#12 interdiff.txt3.56 KBFabianx
#12 implement_bigpipe-2469431-11.patch40.28 KBFabianx
#13 interdiff.txt6.94 KBFabianx
#13 implement_bigpipe-2469431-13.patch43.62 KBFabianx
#17 interdiff.txt7.71 KBFabianx
#17 implement_bigpipe-2469431-17.patch45.95 KBFabianx
#19 interdiff.txt2.68 KBFabianx
#19 implement_bigpipe-2469431-19.patch46.03 KBFabianx
#21 interdiff.txt2.23 KBFabianx
#21 implement_bigpipe-2469431-21.patch45.88 KBFabianx
#28 implement_bigpipe-2469431-28.patch41.77 KBFabianx
#59 interdiff.txt1.42 KBFabianx
#59 bigpipe_for_auth_users-2469431-59.patch42.35 KBFabianx
#62 interdiff.txt3.47 KBFabianx
#62 implement_bigpipe-2469431-61.patch21.66 KBFabianx
#63 interdiff.txt3.35 KBFabianx
#63 bigpipe_for_auth_users-2469431-63.patch41.9 KBFabianx
#74 bigpipe_for_auth_users-2469431-74--based-on-render-strategies.txt13.76 KBFabianx
#74 bigpipe_for_auth_users-2469431-74--proposal-for-ajax-page-state-follow-up.txt3.34 KBFabianx
#74 bigpipe_for_auth_users-2469431-74.patch28.18 KBFabianx
#81 interdiff.txt2.76 KBFabianx
#81 bigpipe_for_auth_users-2469431-81.patch25.99 KBFabianx
#82 bigpipe_for_auth_users-2469431-82--on-top-of-render-strategies.txt14.01 KBFabianx
#82 bigpipe_for_auth_users-2469431-82.patch28.42 KBFabianx
#85 interdiff.txt12.24 KBFabianx
#85 bigpipe_for_auth_users-2469431-84--rs.txt17.24 KBFabianx
#85 bigpipe_for_auth_users-2469431-84.patch32.02 KBFabianx
#87 interdiff.txt6.29 KBFabianx
#87 bigpipe_for_auth_users-2469431-87--rs.txt20.5 KBFabianx
#102 bigpipe_for_auth_users-2469431-102.patch20.49 KBFabianx
#103 interdiff.txt21.9 KBFabianx
#103 bigpipe_for_auth_users-2469431-103--PoC--do-not-test.patch34.08 KBFabianx
#105 bigpipe_for_auth_users-2469431-105--PoC-do-not-test.patch34.24 KBWim Leers
#105 interdiff.txt1.49 KBWim Leers
#106 bigpipe_for_auth_users-2469431-106--PoC-do-not-test.patch34.5 KBWim Leers
#110 bigpipe_for_auth_users-2469431-110--PoC-do-not-test.patch34.76 KBWim Leers
#110 interdiff.txt1.13 KBWim Leers
#124 bigpipe-2469431-124-PoC-do-not-test.patch34.68 KBWim Leers
#126 bigpipe-2469431-126-PoC-do-not-test.patch34.36 KBWim Leers
#126 interdiff.txt3.28 KBWim Leers
#127 interdiff.txt412 bytesWim Leers
#128 bigpipe-2469431-128-PoC-do-not-test.patch33.92 KBWim Leers
#128 interdiff.txt975 bytesWim Leers
#129 bigpipe-2469431-129-PoC-do-not-test.patch34.06 KBWim Leers
#129 interdiff.txt2.56 KBWim Leers
#129 interdiff-128.txt569 bytesWim Leers
#130 bigpipe-2469431-130-PoC-do-not-test.patch34.59 KBWim Leers
#130 interdiff.txt1004 bytesWim Leers
#131 bigpipe-2469431-131-PoC-do-not-test.patch30.43 KBWim Leers
#131 interdiff.txt7.81 KBWim Leers
#132 bigpipe-2469431-132-PoC-do-not-test.patch32.44 KBWim Leers
#132 interdiff.txt2.7 KBWim Leers
#135 bigpipe-2469431-135-PoC-do-not-test.patch32.86 KBWim Leers
#135 interdiff.txt8.86 KBWim Leers
#136 bigpipe-2469431-136-PoC-do-not-test.patch33.71 KBWim Leers
#136 interdiff.txt3.39 KBWim Leers
#141 bigpipe-2469431-141-PoC-do-not-test.patch41.54 KBWim Leers
#141 interdiff.txt17.1 KBWim Leers
#144 bigpipe-2469431-144-PoC-do-not-test.patch41.96 KBWim Leers
#144 interdiff.txt1.71 KBWim Leers
#148 bigpipe-2469431-148-PoC-do-not-test.patch41.95 KBnod_
#148 interdiff.txt6.28 KBnod_
#152 bigpipe-2469431-152-PoC-do-not-test.patch41.97 KBWim Leers
#152 interdiff.txt4.67 KBWim Leers
#155 bigpipe-2469431-155-PoC-do-not-test.patch40.67 KBWim Leers
#155 interdiff.txt5.16 KBWim Leers
#159 bigpipe-2469431-159-PoC-do-not-test.patch41.57 KBWim Leers
#159 interdiff.txt8.44 KBWim Leers
#160 bigpipe-2469431-160-PoC-do-not-test.patch46.17 KBWim Leers
#160 interdiff.txt7.19 KBWim Leers
#161 bigpipe-2469431-161-PoC-do-not-test.patch46.96 KBWim Leers
#161 interdiff.txt1.61 KBWim Leers
#162 bigpipe-2469431-162-PoC-do-not-test.patch47.44 KBWim Leers
#162 interdiff.txt1.42 KBWim Leers
#168 bigpipe-2469431-168-PoC-do-not-test.patch47.36 KBWim Leers
#168 interdiff.txt1.23 KBWim Leers
#171 bigpipe-2469431-171-PoC-do-not-test.patch50.73 KBWim Leers
#171 interdiff.txt7.34 KBWim Leers
#172 bigpipe-2469431-172-PoC-do-not-test.patch50.75 KBWim Leers
#172 interdiff.txt1.47 KBWim Leers
#173 bigpipe-2469431-173-PoC-do-not-test.patch51.32 KBWim Leers
#173 interdiff.txt2.7 KBWim Leers
#174 bigpipe-2469431-174-PoC-do-not-test.patch50.93 KBWim Leers
#174 interdiff.txt4.52 KBWim Leers
#175 bigpipe-2469431-175-PoC-do-not-test.patch52.18 KBWim Leers
#175 interdiff.txt6.2 KBWim Leers
#179 bigpipe-2469431-179-PoC-do-not-test.patch52.35 KBWim Leers
#179 interdiff.txt3.8 KBWim Leers
#180 bigpipe-2469431-180-PoC-do-not-test.patch58.71 KBWim Leers
#180 interdiff.txt23.54 KBWim Leers
#181 bigpipe-2469431-181-PoC-do-not-test.patch65.49 KBWim Leers
#181 interdiff.txt14.75 KBWim Leers
#182 bigpipe-2469431-182-PoC-do-not-test.patch66.58 KBWim Leers
#182 interdiff.txt4.1 KBWim Leers
#183 bigpipe-2469431-183-PoC-do-not-test.patch66.59 KBWim Leers
#183 interdiff.txt17.49 KBWim Leers
#184 bigpipe-2469431-184-do-not-test.patch66.66 KBWim Leers
#184 interdiff.txt1.86 KBWim Leers
#222 Drupal_system_Tests_Module_InstallUninstallTest-209-2.txt547.51 KBeffulgentsia
#222 Drupal_system_Tests_Module_InstallUninstallTest-61-2.txt545.75 KBeffulgentsia
#223 dbg-do-not-test.patch1.27 KBWim Leers
#223 dbg-big_pipe_nojs-do-not-test.patch1.03 KBWim Leers
#224 bigpipe-output.png196.03 KBswentel
#224 big_pipe-2469431-223.patch119.29 KBswentel
#224 2469431-223-interdiff.txt2.58 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Issue summary: View changes
FileSize
27.17 KB

And here is a first proof-of-concept patch that I ported from render_cache-7.x-2.x

Note: BigPipe was hacked in here, but the general placeholder support for cacheability works also without this.

I will upload a separate patch for that later.

However currently #pre_render is still used, which cannot be easily cached (it can but serialization data is huge), but once we use #pre_render_cache we can easily do that.

--

This should not be tested via testbot, yet.

--

Edit: This has a hacked system-branding block to show the current users name and also take 2 seconds to show some effect.

Fabianx’s picture

Status: Active » Needs work
FileSize
27.17 KB

Uploading a .patch file as else 'Test on simplytest.me' button does not appear in dreditor, but please do not test this.

EDIT:

Does not work on simplytest.me, likely due to a varnish load balancer or any other system component that does not allow streaming.

To test locally

- Apply patch
- Add system branding block to left sidebar at the bottom of the region
- Disable page cache to test with an anonymous user

Fabianx’s picture

And here is a next patch that shows how this would work with #pre_render_cache.

Note:

- We either need a dedicated interface on the block plugins for that.
- Or a $entity->isDirty() flag

e.g. the main content block cannot be #pre_render_cached, because it receives it's data via ->setContent().

This will need some thought.

Maybe a better way is to use:

$elements = [
  '#cache' => [...].
  '#pre_render_cache' => [...],
  '#pre_render' => [...],
  '#other_items' => ...,
];

etc. instead of always going via #pre_render_cache, which is slower as the entity needs to be reloaded ...

--

Attached patch still works, but is not for testbot.

mikeytown2’s picture

Is this essentially ESI in core with some additional benefits? If using HHVM will it allow for a single request to be parallel-ed http://docs.hhvm.com/manual/en/hack.async.php

Fabianx’s picture

Issue tags: -ux +user experience
FileSize
4.08 KB
7.74 KB
33.08 KB

This is not ESI in core, but a pre-requisite for proper support of ESI and yes, I immediately thought of httprl and parallel execution, too.

Theoretically could dump $request AND the $render_cache_array to a temporary entry, use a HttpMiddleware to load the original request from the temp store, execute it for context setting, etc. (authentication, ...) but set a flag with the $render_cache data then at route subscriber, just execute the $render_cache_array and return the fragment according to the rendering method.

Besides having to cleanup the temporary entries that should be 'simple' to implement once BigPipe and placeholdering works properly.

For ESI (from Varnish) to work correctly, the cache contexts (output as part of X-Drupal-Cache-Contexts) will need to be replaced inside varnish and added to the hash using a restart strategy (but that is another issue, but this issue is my playground sooooo ;)).

--

To the issue, setting both #pre_render and #pre_render_cache works great and solves the concerns for most run-time performance.

I kinda like the compact / extract pattern (kudos to douggreen for using it).

tadityar’s picture

Status: Needs work » Needs review

Should be Needs Review?

Status: Needs review » Needs work

The last submitted patch, 5: implement_bigpipe-2469431-4.patch, failed testing.

Fabianx’s picture

Now we are only missing bubbled max-age values and we should incorporate this into cacheGet / cacheSet directly as the data really does not need to be processed more than needed.

Doing that now:

- cacheGet($elements) => cacheGet($elements, $recursive = FALSE)
- Ensuring we only do one #cache_redirect (2 levels is maximum)
- Move placeholder code from after cacheGet to cacheGet itself and return as $cached
- This allows then to check isset($cache_redirect['#cache_placeholder']) to support bubbling step 1.

Still to do, too:

- Add checkPlaceholders() functionality to
a) preload placeholders via cacheGetMultiple()
b) ensure all placeholders can be resolved (either cached or #pre_render_cache is set)

Example for that:

Consider a block cacheable by user only and either via high-frequency cache context or the block setting #cache_placeholder = TRUE in its cache metadata BUT due to various reasons not #pre_render_cache-able (e.g operating on a prepared part of the main request) and hence unsetting #pre_render_cache in its cache metadata.

Now consider a page cached with a placeholder for said block.

The placeholder looking like:

$placeholder = [
  '#cache' => [
    'keys' =>  ['block', 'my_user_block']
    'contexts' => ['user'],
    'max-age' => Cache::PERMANENT,
  ]
];

Once the user block has been cached once, any other cached page the user visits will be able to resolve the placeholders and hence give the user the full cached page with placeholders replaced.

Therefore #pre_render_cache is great for BigPipe, but not necessary to ensure we can cache larger chunks with #smartcache by placeholdering other things.

Fabianx’s picture

FileSize
5.96 KB
35.9 KB

This is starting to get really nice!

The code in cacheGet gets a little more complex, but the majority is if checks inlined for performance reasons.

We can now safely render a placeholder if the redirect stores the information that a placeholder is needed!

That is part 1 of the bubbling, now onto part 2.

Fabianx’s picture

FileSize
5.14 KB
39.47 KB

Bubbled max-age = 0 now also works nicely.

The #cache_placeholder entry in $cache_redirect works great!

What is a little problematic right now is that the initial page load takes 4s as the placeholder is rendered twice. (due to bubbled max-age coming just after the execution).

Obviously thats not necessary as the data already exists. The next step is to make that data available when rendering a placeholder.

Fabianx’s picture

That was simpler than I thought :).

This patch introduces a class level variable placeholderData and pushed that placeholderData into the variable, then if its set uses the 'static rendering strategy'.

Note: That while for BigPipe it is appreciated that if the data is cached we show it directly, for ESI we would always need to replace all placeholders, hence needing rendering strategies as explained in #2349011: Support placeholder render strategies so contrib can support BigPipe, ESI… already.

For the proof-of-concept this is fine however.

--

Next up:

- Multiple cacheGet when retrieving placeholders from cache and storing the data in $this->placeholderData as well.

Related:

- To implement the BigPipe render strategy in a clean way we want a RenderResponse, which has the #cache metadata of the HtmlContentRenderer.
- This should also fire an onBeforeSend / onAfterSend event and overwrite the send() method with that.
- BigPipe can re-act on this event and take a look if there are placeholders left to render, then send them in a streamed way.

Fabianx’s picture

FileSize
3.56 KB
40.28 KB

Patch for #11

Fabianx’s picture

FileSize
6.94 KB
43.62 KB

And the "holy grail" of gradual performance improvement has been reached! :-D

With the last demo (setup site branding, move to sidebar, disable page cache, load /) there is a really nice behavior:

- First load, the whole site branding block is loaded via BigPipe and the page takes around 150 ms to first byte and 2.5 secs until the block appears (due to the 2 sec delay in building the block and 0.5 secs in building the
dynamic data).
- Second load, only the dynamic portion is loaded via BigPipe as the site branding block for _this_ user is already cached! Hence 150 ms to first byte and 0.5 seconds until the dynamic portion of the page appears.

How this works

- The site branding block has a dynamic uncacheable portion now, which takes 0.5 secs to build.
- We give a hint to the system via alterBuild() that we want to render this as a placeholder and the user cache context is bubbled up from below (we could also set it our own - does not matter).
- We add the dynamic portion via #pre_render_cache / #cache + max-age = 0 pattern. The only thing needed is to also set a 'cache key' - even though this is uncacheable (this is by design).

The first time the system branding block placeholder is not in the cache and hence not resolvable, therefore we render the whole block and the dynamic portion is also rendered in the BigPipe stage (needed a global to enforce that for now - will need to think about something to not nest things ...).

The second time we know the placeholder is cached already, store it temporarily and then render it via processPlaceholders in the final renderRoot() call.

Then the remaining placeholder for BigPipe to render is the dynamic one and it all works!

Patch for testing the proof-of-concept attached.

Note how simple it is for a developer to make a site fast when he knows which blocks can be cached how and what placeholders should be used for what portion of the page.

--

Next is testing a cached nested placeholder that is not #pre_render_cache-able and then except cleanup and tests the placeholdering part of this is done and work on proper JS for BigPipe can begin and the placeholder patch can be moved to its own issue as the concept and potential has been proven enough and even now this can make smartcache _way_ smarter.

Just 40 kB, too!

mikeytown2’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: implement_bigpipe-2469431-13.patch, failed testing.

pounard’s picture

This seems very nice, after reading your comments and you patch I have still have a few questions:

  • For JS disabled browsers, the content may not be included this way, so this should be checked one way or another, can this be done easily?
  • This should remain an optional behavior I think, some sites don't fit with the need of having a bigpipe, this is surely a need for sites which have a lot of logged-in users, but not for pure CMS sites.
  • Is that possible that each placeholder be sent one after the others? (you send the body first into the stream, then all rendered placeholders at once, each placeholder could be sent, flushed, one after the other)
  • In order to be replaceable by an ESI gate, parts should probably be also exposed via a router URL? This would also allow AJAX fetch, but right now I might see a problem: not all modules will actually expose a router URL taking into account the page context.
  • To make it even faster, the headers could be sent before rendering the body so that the broswers starts downloading assets etc? But I don't know if this is doable because in order to know what JS/CSS to include you may need to generate almost everything.

Note that those questions are probably a bit off-topic since this page is about delaying the rendering of elements you cannot really get outside of the page context without extensively patching core or the modules providing them.

Aside of that, I don't know the D8 render API, which seems quite complicated to follow, took me a few hours to read the whole cache and render API and that's not easy to deal with:

  • Such behavior and the whole strategy you put in place to delay cache misses will be quite hard to understand if this got merged into core. Is there a way to delegate this to some decorator or visitor object?
  • That's a nice a proof of concept, but implementing it without being invasive will probably much harder?
Fabianx’s picture

FileSize
7.71 KB
45.95 KB

Okay, next patch:

In this patch the final scenario is tested what happens when something can be cached as part of a larger structure and varied on something but not re-created without that structure being executed first. (i.e. think of a view or another complex object).

As can be seen from the interdiff:

- Added function renderPlaceholder() to keep the #cache_no_placeholder internally and allow BigPipe render strategy to make use of already calculated placeholder data.
- Ensure no placeholder is created when checkPlaceholders() calls $this->cacheGet()
- Ensure that when a placeholder is asked for that this is stored in the cache entry, regardless if this is a cache redirect or not. (This happens when $cid == $pre_bubbling_cid) to avoid having to check if where we want to store this already a #cache_placeholder exists (this saves one cache_get call).
- Ensure that both cache_redirect and cached_element is checked for a #cache_placeholder flag.

Finally and this was the most tricky one:

- Simulate bubbling even for data stored statically, but rendered later in a #cache_no_placeholder context, so that cacheGet / cacheSet work properly.

--------

The new demo:

A user cacheable site branding block with:

- a dynamic element using #pre_render_cache and max-age=0
- a route cacheable element that uses a normal #pre_render as it needs the $this (scenario), which opts in to #cache_placeholder in the #pre_render callback

After caches are warm only the dynamic portion is loaded via BigPipe, but if you now clear just the 'site_branding_block_custom' cache tag, then you only need to render the site branding block once via BigPipe, afterwards it will be loaded from cache, when the route cacheable entry placeholder has already been calculated!

(drush ev '\Drupal\Core\Cache\Cache::invalidateTags(['site_branding_block_custom']);')

Fabianx’s picture

This seems very nice, after reading your comments and you patch I have still have a few questions:

Thanks!

For JS disabled browsers, the content may not be included this way, so this should be checked one way or another, can this be done easily?

Yes, we have a 'js' cookie for that. Another concern is SEO. Yes, this needs some thought.

This should remain an optional behavior I think, some sites don't fit with the need of having a bigpipe, this is surely a need for sites which have a lot of logged-in users, but not for pure CMS sites.

Agree, it should be able to be turned off.

Is that possible that each placeholder be sent one after the others? (you send the body first into the stream, then all rendered placeholders at once, each placeholder could be sent, flushed, one after the other)

This is already implemented in exact this way :).

In order to be replaceable by an ESI gate, parts should probably be also exposed via a router URL? This would also allow AJAX fetch, but right now I might see a problem: not all modules will actually expose a router URL taking into account the page context.

This is part of the larger problem space, but I have a solution for that, this is my ESI (/ AJAX) plan:

- Store data to temp store based on hash($render_cache_array) with cache tags
- Have route /fragment/hash (with internal dumb page cache off), but also send original request
- Have signed cookie or header that is hash of the hash including a timestamp (so it expires) (similar to ESI module)
- Also all the cache context handling needed for variations of that url (internally - explained above already)
- In case the placeholder is not replaceable or the validation failed, execute the original request as fallback, but then only return the now generated placeholder
As placeholder hashes don't change as they are based on the cache information and/or pre_render_cache information, usually a placeholder can be updated.

Needs some security thought too.

Also see http://wimleers.com/talk-render-caching-drupal-7-and-8/#/4/7

To make it even faster, the headers could be sent before rendering the body so that the broswers starts downloading assets etc? But I don't know if this is doable because in order to know what JS/CSS to include you may need to generate almost everything.

That is exactly what we do here, slow loads are deferred to later, but yes a strategy to send everything up to the next to be replaced placeholder is possible, too. Then also have a placeholder in the footer for the scripts and send a new aggregate. Needs some experiments.

Aside of that, I don't know the D8 render API, which seems quite complicated to follow, took me a few hours to read the whole cache and render API and that's not easy to deal with:

I hope Wim's documentation on cacheability/8 was helpful then :).

Such behavior and the whole strategy you put in place to delay cache misses will be quite hard to understand if this got merged into core. Is there a way to delegate this to some decorator or visitor object?

This is not possible and would only degrade performance. The functionality of placeholdering and #cache_redirect is encapsulated in the cacheSet / cacheGet layer though so we could decouple that part at least: #2466585: Decouple cache implementation from the renderer and expose as renderCache service

This is the only way we can avoid that smart cache needs to be varied by _all_ the things on the page, which can be a lot and have way less route-level cacheability that way. #cache_redirect and context bubbling was about making rendering correct, placeholdering is for ensuring the cache granularity is manageable.

e.g. consider a site with a 'user' block 1, consider 100'000 pages and 100 users and smart cache, then you end up with 100 variations of 100'000 pages. So if every user visited every page => 10'000'000 cache entries based on route+user.

With placeholdering in its simplest form for that user block (with hook_page_alter gone the impact on contrib should be not there at all) and not even using #pre_render_cache parts of this patch:

100 entries for user_block:%user
100'000 entries for smart cache based only on the route and parameters: smartcache:%route_with_parameters

Way better cache re-use for users, too.

I think your main concern is the automatic placeholdering of max-age = 0 and high-frequency cache contexts and I agree that that should be a follow-up and proper discussion. A first step is to make placeholdering via opt-in (e.g. #cache_placeholder = TRUE) on a render-cacheable element possible. (This is all in this patch already, I just need to delete lines to reach that.)

--

Yes, it is complicated and yes it is hard to understand, but we try to do something here that - to my knowledge - no one (except me in render_cache in D7 in prototype stage) has done so far, but which gives _amazing_ results in cacheability.

And cacheability and performance is something we badly need for Drupal 8 to be a success performance wise.

As a developer (unless you try to render something early and don't use $renderer->renderPlain()) you should not be affected at all by this change. It just gives you more possibilities.

That's a nice a proof of concept, but implementing it without being invasive will probably much harder?

Not hard at all, we only change internals and because blocks and most things use the #pre_render pattern, they are never rendered in any render array anyway during their life time, so it does not matter if there is a placeholder or the actual block markup.

So at least for block-level granularity we could safely enable placeholdering IMHO.

--

This is the main use case of the placeholdering, way easier support for BigPipe, ESI, ajax rendering, etc. is just a very nice side effect (and the most impressive demo!) ...

-----

Next up:

- Create a patch with just the placeholdering parts in a different issue
- Explore moving the BigPipe to a RenderStrategy class, register via service collector and have a RenderStrategyManager
- Change $is_root_call to a $flags element, keep BC (for === TRUE | FALSE) and add Renderer::USE_RENDER_STRATEGIES flag and remove the global declaration

Fabianx’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
46.03 KB

And a patch removing the max-age checks for now and also restoring the site branding block to its original function.

That means the placeholdering is purely opt-in now.

This has the potential to be green :).

Lets see what I have missed.

Status: Needs review » Needs work

The last submitted patch, 19: implement_bigpipe-2469431-19.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
45.88 KB

This should be green (hopefully).

mikeytown2’s picture

Great work!

Looking at the jquery requirement and I think that's a todo for another issue. The 2 options for javascript esi parsing from what I can tell are these 2
https://github.com/MrSwitch/esi
https://github.com/slashrsm/jquery_esi (derivative of drupal's esi code)

Fabianx’s picture

#22: Thanks! We will likely use the core AJAX framework for now (but stream the ajax commands) as it handles assets, etc. natively and attaches behaviors.

Also any optimization we do in the AJAX framework, we can leverage via BigPipe directly.

YesCT’s picture

Issue summary: View changes
Anonymous’s picture

Will it be possible to retrieve only a portion of a page with this? Like a certain block from a certain page(so only the block would get rendered(neither the post render cache would get triggered) and no other part of the page)?

Fabianx’s picture

#25: Yes, but only with the block has the capability to be rendered without any request knowledge, i.e. a block having #pre_render_cache (or whatever we call it) set.

Wim Leers’s picture

Issue tags: +Needs reroll

I had to go all the way back to 9c99c595af6bf4d23fa6378e5035043dcac3bc14 (April 15) to be able to apply this patch. Now working on a thorough review, but a reroll would definitely be welcome.

Fabianx’s picture

Issue tags: -Needs reroll
FileSize
41.77 KB

We have been active, here is the quick re-roll.

No interdiff, because no changes.

Wim Leers’s picture

Thanks!

While I review this, it'd be great if you could already explain how you plan to get this in: as one big patch, or which parts could go in as separate patches?

Status: Needs review » Needs work

The last submitted patch, 28: implement_bigpipe-2469431-28.patch, failed testing.

Fabianx’s picture

Issue tags: +Needs reroll

TL;DR version of the below:

  1. There are 4 use cases we need to support
  2. There are 2 use cases that are easy to support (#cache_placeholder == TRUE is known before calling cacheGet)
    1. There is 1 use case that does not need any changes to consumers of the render API
    2. There is 1 use case that does need consumers to set #pre_render_cache
  3. We need render strategies to support BigPipe, ESI, ...
  4. We need a BigPipeResponse class and need to support the AJAX framework
  5. We want to support the complicated use cases, too

Sure,

so the plan to split this up is based on use cases and for the start make placeholdering opt-in via a #cache_placeholder property that lives right next to a #keys property (it is _not_ bubbled), then later support max-age =0 > automatic placeholdering.

I analyzed 4 possible ones, which fall in 2 categories, all other things are variations of that:

Cacheable, but not re-creatable dynamically:

  • 1a. A block cacheable by 'user' which is configured with $pre_bubbling_elements['#cache']['contexts'] == ['user'] and #cache_placeholder => TRUE
  • 1b. A block cacheable by 'user' where the 'user' cache context and #cache_placeholder => TRUE information bubbles up.

#pre_render_cache dynamically re-creatable out of context

  • 2a. A dynamic block which is configured with $pre_bubbling_elements['#cache']['max-age'] == 0 (and #cache_placeholder => TRUE)
  • 2b. A dynamic block which where the $pre_bubbling_elements['#cache']['max-age'] == 0 and #cache_placeholder => TRUE information bubbles up.

To achieve supporting that use cases, 1. is simpler to implement as we don't need a new property for it.

Note: The b) cases can be follow-up as for them the logic in cacheSet is more difficult, the a) case is simple and only changes in cacheGet are needed.

- Step 1: Add bubbleable placeholders property - now with CacheableMetadata I am less sure that it should live in ['#cache']['placeholders'] though, but it needs to be bubbleable. - Discuss!

- Step 2: Implement the cacheable 1a case, that is mainly changing cacheGet / cacheSet to check if we want to render a placeholder and add a renderPlaceholders() function. This also needs to add the little stack change that the stack is fully populated into the cacheable build then emptied and then the element pushed again. (see diff)

All complicated put #cache_placeholder in the redirect cache item logic does not need to be supported here as its the b) case which makes this tricky.

Alone this makes it possible to make smartcache smarter, as it then varies by route and a placeholder'ed block is added as a placeholder into the smartcache.

If smartcache retrieves from cache and the placeholder is not cached, this is treated as a cache miss. (because we have no way to re-create it at this stage - BUT it would have been a cache miss with the user varied smartcache, too so we only win for cases that its cached, and loose nothing in other cases.)

That was the gist of my idea in the original gist ;).

Step 1+2 should be done together so that Step 1 is not useless core junk.

The next steps can be other issues that can be worked on even in parallel:

- Step 3: Implement #pre_render_cache and convert our block manager to support it (like in this patch).

Note: #pre_render_cache is _optional_ information in addition to #pre_render. It is only used when #pre_render is absent, one can choose to only use #pre_render_cache, but in the case of the block builder it wants to run an alter hook on the built render array (with #cache and #block) and therefore #pre_render_cache kinda populates with the information needed for #pre_render.

That implements 2.a.

Missing from this patch still are:

- Step 4: Add render strategies via a render strategy manager so that placeholders are not just replaced in renderPlaceholders, but also BigPipe, ESI, Static, Cached, ... and allow to set usage or not usage of the render strategy manager. In other words if something calls renderPlain() for an email they surely don't want ESI tags, if something is not a root call, we also don't want ESI tags, but if the HtmlRenderer calls that we do want ESI tags and possibly also some #big_pipe_placeholders in the returned $build.

Possibly to also just not deal with the placeholders at all in the renderer (when the flag is set) and call the RenderStrategyManager from the HtmlRenderer directly.

Render Strategies use the Chain-of-Responsibility pattern and each Strategy says which placeholders it took care of.

- Step 5 (depends on render strategies): Add a BigPipeResponse class that knows both the placeholders and the raw content and overwrites the send() method.
- Step 6 (needs step 5): Make the BigPipe render strategy use the AJAX framework so that assets, etc. are supported.

In this patch, but a little more complicated:

- Step 7: Discuss / Implement the 1/2 b) cases and think about adding auto-placeholdering for max-age=0 and "high-frequency" cache contexts.

Optional and just stream of thought:

- Step 8: If would be nice to have if some additional information could be given when asking for a placeholder, like:

#cache_placeholder = TRUE, #cache_placeholder_info = [ 'strategies' => ['esi', 'bigpipe']] that is stored alongside when creating the placeholder.

Crell’s picture

Reminder: When designing a swappable system, there's an adage that "you don't know it works until you have 3 implementations." It's proven true in my experience. The potential implementations I can see as "easy" targets (all things being relative) would be

1) One big HTML page with no placeholders (the default)
2) Varnish ESI tags
3) In-process replacements using inlined Ajax replacement commands (aka BigPipe)
4) In-browser replacement using the *same* inlined Ajax replacement commands (probably Drupal custom, but so what?)

(For 3 and 4, we should use the existing AjaxResponse framework. It should work for both cases at the same time. If it doesn't, it's a bug.)

I don't know which of those are the ideal canaries to code against, but keeping at least 3 in mind while designing will lead to the most robust solution. (That was definitely the case for the DBTNG work in Drupal 7, and various other places.)

Fabianx’s picture

#32:That is great advice and you are correct.

I even have 5 use cases designed:

1. Normal HTML (the default) - trivial

2.a BOOT (in D8: Kernel Middleware, only used for replacing already cached data)
2.b PRE-DRUPAL: Varnish / NGINX with memcache / redis plugin and some custom code

3.a ESI: ESI tags with cache tag aware Varnish
3.b ESI-REVALIDATE: ESI with non-cache tag aware Varnish (ESI-REVALIDATE)

4.a AJAX: Normal ajax requests using UUID URLs
4.b AJAX-LOCALSTORAGE-REVALIDATE: Storing cache tags locally and re-validating tags, possibly against a DrupalKernel middleware

5. BIGPIPE

The general plan can be found in our presentation http://wimleers.com/talk-render-caching-drupal-7-and-8/#/4/1.

Note that I speak of a concept similar to cache contexts without knowing at that time that Drupal 8 already has what I need :-D.

Wim Leers’s picture

I'm still working on my big review. But big +1 to the rationale behind #32.

I'd go even further: I think this patch needs to grow to include:

  1. comprehensive test coverage
  2. auto-placeholdering (i.e. discover the parts that are "too dynamic too cache" and which should thus be served by BigPipe/ESI/…)
  3. use auto-placeholdering to get rid of code that does it manually: at least the comment form on nodes, the statistics block and perhaps node/comment links
  4. cleanly switch between "Single Flush/no placeholders" (HEAD), BigPipe and something else (e.g. ESI-in-JS, per #22)

i.e. first prove that 1) it works, 2) that the architecture is comprehensible (for the latter point, I'm tagging this for an IS update).

Fabianx also mentioned mixing "render strategies", i.e. use ESI for some placeholders and BigPipe for others. I'm not sure that super advanced use case is necessary. I think it'd be fine to defer that to a follow-up, or Drupal 8.x.0. But everything else needs to be proven in one big, coherent patch, before we start splitting it up into child issues.

But! No worries, Fabianx. I'm here to help! I will help you get there. But I do agree with Crell that it's important we first prove the "integration viability". You proved it is viable at all. But now we need to prove that when fully integrated with Drupal, that it actually works, and that it doesn't complicate the DX.

Wim Leers’s picture

Issue summary: View changes

Here's a first pass at updating the IS. Expect more updates throughout the day.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

An interesting alternative/additional possibility is CSI (Client-Side Includes) + sessionStorage: anything that is per user or session, which is considered too dynamic, but which can actually be cached in sessionStorage

(Not important, but just wanted to mention that, so that we don't forget about the interesting potential there. That could easily be Drupal 8.x.0 material: it'd just be a feature addition.)

plach’s picture

Sorry if you already provided an answer for this, but I was wondering whether having multiple rendering strategies would help supporting user agents having JavaScript disabled or not available. I think somewhere Fabian referred to a cookie where saving whether the UA has JS enabled, however this would mean that on the very first request we need to use strategy 1 (regular HTML), since the cookie would not be set. I was also wondering whether it would make sense to make the default strategy configurable, so that sites that simply do not support UAs with JS disabled can have the fast behavior for the first request, which can very well be the one that requires a fast response most.

It would also be great if the IS could outline whether/how rendering strategies affect the HTML markup architecture and the progressive enhancement methodology.

Wim Leers’s picture

Here is an initial review. Only looking at the big picture, not the details. My first goal is to make this patch understandable, to understand why it makes the changes it makes.

  1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -157,17 +157,94 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +        // Split it up in various chunks.
    +        $split = '<!-- X-RENDER-CACHE-BIG-PIPE-SPLIT -->';
    +        if (strpos($markup, $split) === FALSE) {
    +          $split = '</body>';
    +        }
    +        $page_parts = explode($split, $markup);
    

    Allowing templates to specify where to do the split is definitely not essential. Let's KISS and just default to splitting at the closing body tag.

  2. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -157,17 +157,94 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +        if (count($page_parts) !== 2) {
    +          throw new \LogicException("You need to have only one body or one <!-- X-RENDER-CACHE-BIG-PIPE-SPLIT --> tag in your html.html.twig template file.");
    +        }
    

    Then this can be simplified too.

  3. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -157,17 +157,94 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +          // Check if the placeholder is present at all.
    +          if (strpos($markup, $placeholder) === FALSE) {
    +            continue;
    +          }
    

    Why do we even need to check this? If the placeholder has bubbled, then it must also exist in the markup.

  4. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -157,17 +157,94 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +          if ($placeholder_markup == '') {
    +            continue;
    +          }
    

    Why not KISS and not special-case this? Let's just send the necessary JS to replace this too?

  5. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -157,17 +157,94 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +          // @todo Output as AJAX response so we can re-use the AJAX code.
    ...
    +        // Now that we have processed all the placeholders, attach the behaviors
    +        // on the page again.
    +        print $behaviors;
    

    This todo will also allow us to get rid of that second attachBehaviors() call :)

  6. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -157,17 +157,94 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +          print <<<EOF
    +<script type="text/javascript">
    +jQuery('#' + "$placeholder").replaceWith($html);
    +</script>
    +EOF;
    

    This is not yet handling the loading of additional attachments! Note that this needs to exclude already-loaded assets.

  7. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -74,6 +74,13 @@ class Renderer implements RendererInterface {
       /**
    +   * Static cache of rendered placeholder data.
    +   *
    +   * @var array
    +   */
    +  protected $placeholderData = [];
    
    @@ -128,6 +135,21 @@ public function renderPlain(&$elements) {
    +    if (isset($this->placeholderData[$placeholder])) {
    +      // Replace $elements with the statically cached data.
    +      $elements = $this->placeholderData[$placeholder];
    +    }
    

    When would we ever have multiple occurrences of the same placeholder?

    Feels like a premature optimization/unnecessary complication.

  8. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -193,14 +215,24 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // Two-tier caching: track pre-bubbling element for later
    +    // comparison.
    +    // @see ::cacheGet()
    +    // @see ::cacheSet()
    +    $pre_bubbling_elements = $elements;
    
    @@ -213,12 +245,16 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    -    // Two-tier caching: track pre-bubbling elements' #cache for later
    -    // comparison.
    -    // @see ::cacheGet()
    -    // @see ::cacheSet()
    -    $pre_bubbling_elements = [];
    

    Why move this?

  9. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -213,12 +245,16 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    -    $pre_bubbling_elements['#cache'] = isset($elements['#cache']) ? $elements['#cache'] : [];
    

    And why delete this?

  10. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -378,6 +415,11 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // All data is now in $elements, so discard stack as cacheSet can change this.
    +    // @todo Simplify this!
    +    static::$stack->pop();
    +    static::$stack->push(new BubbleableMetadata());
    

    I don't understand this.

  11. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -520,15 +610,18 @@ protected function processPostRenderCache(array &$elements) {
    +  protected function cacheGet(array $elements, $is_recursive = FALSE) {
    

    s/$is_recursive/$is_redirect/, to avoid confusion with ::render(…, $is_root_call).

  12. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -536,12 +629,124 @@ protected function cacheGet(array $elements) {
    +    $placeholder['#markup'] = '<div id="' . $pholder_id . '"></div>';
    

    Why use <div>s as placeholders? Wouldn't HTML comments be safer? If not HTML comments, then why not a custom tag?

    And most importantly: this is BigPipe-specific. This actually needs to call out to the selected Render Strategy service, so that it can choose how to render this. E.g. HTML comments for BigPipe, <esi:include> for ESI.

  13. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -582,6 +787,52 @@ protected function cacheSet(array &$elements, array $pre_bubbling_elements) {
    +    // Placeholdering at work, check if we can placeholder.
    +    if (!isset($pre_bubbling_elements['#cache_no_placeholder']) && $pre_bubbling_cid && ($cid || isset($pre_bubbling_elements['#pre_render_cache']))) {
    ...
    +        $elements = $this->createRenderCacheArrayPlaceholder($pre_bubbling_elements, $data);
    

    Why does placeholder creation happen inside the function that is about creating render cache items?

    This means that placeholders are only created for subtrees that asked to be render cached. Why do that?

    This is for me by far the biggest stumbling block WRT understandability.

  14. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    index 216dfa8..6b3ad60 100644
    --- a/core/modules/block/src/BlockViewBuilder.php
    
    --- a/core/modules/block/src/BlockViewBuilder.php
    +++ b/core/modules/block/src/BlockViewBuilder.php
    
    +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -73,19 +57,75 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +        '#pre_render_cache' => [
    +          $class . '::preRenderBlock' => compact('class', 'entity_type', 'entity_id', 'view_mode', 'langcode'),
             ],
    

    Quite significant changes here; all for this. Definitely needs further scrutiny.

  15. +++ b/core/modules/block/src/BlockViewBuilder.php
    index c5ebaa5..7deff24 100644
    --- a/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    
    --- a/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    

    All of these changes are just for demo purposes. We can happily ignore this for now.

Wim Leers’s picture

The first thing you see in the patch, and the new concept/abstraction that is crucial for this: bubbleable placeholders. #39 already had some questions about that.

Which brings me to the central question:

Why still keep #post_render_cache callbacks if we have "placeholders"?

I think it's clear that "placeholders" is a clearer, more intuitive, significantly easier to understand concept. So why use both, when #post_render_cache is actually being used for replacing placeholders already, though in a much more limited way?

  1. Initially, my answer was: for scenarios that don't involve markup, e.g. attaching an asset library or drupalSettings only.
  2. But then, while working on #39, I realized that the interaction of #post_render_cache + placeholders would quite possibly end up to be too confusing:
    BigPipe
    #post_render_cache callbacks executed first (as part of the initial flush), then in subsequent flushes, placeholders are rendered/replaced
    Single-Flush (as in HEAD)
    #post_render_cache and placeholders are effectively equivalent
  3. I think we should consider removing #post_render_cache. The #attached case for #post_render_cache only has a single use case in core (history_attach_timestamp()), which could also be handled by a placeholder that returns the empty string as #markup, but does set #attached.
    The only case in core for which this is wrong, is setting the "active" class on links for anonymous users, which is done by the SystemController::\Drupal\system\Controller\SystemController::setLinkActiveClass() #post_render_cache callback. But, that's such a very very very special case, and actually, Symfony already has a nice abstraction for it: the FilterResponseEvent. (IMO we should use that mechanism anyway, regardless of this issue.)
    That'd allow us to keep things simple, and in fact, make things simpler compared to HEAD.
  4. The only reason I can think of to keep both: use #post_render_cache for uncacheable-but-light-weight replacements, and placeholders for "meatier chunks of content". But that's subjective. And it's confusing when to use which.
    Can you think of reasons to keep both?
  5. Finally, on a less conceptual, but more technical note: see #39.13: I don't understand why 1) placeholder handling happens in render cache get()/set(), 2) I don't understand why it only supports render subtrees that have ['#cache']['keys'] set.

I think it's crucial that we first flesh out the "placeholders" part of this patch. If we agree that we that we can remove #post_render_cache in favor of the conceptually simpler "placeholders", then I think we could file an issue for doing just that and mark it as a blocker for this issue, but it would be a DX improvement regardless.

Wim Leers’s picture

Fabianx’s picture

Thanks so much for IS updates (that is spot-on) and the review. I will respond to everything in detail later today!

yched’s picture

FWIW, I also find that having both placeholders + #post_render_cache is confusing, and the whole render api + flow of cache metadata + behavior wrt the various rendering strategies doesn't feel completely trivial alrlready.

+ 1 for reducing the number of concepts figure out :-)

Fabianx’s picture

Quick feedback:

- I thought long about it and yes, #post_render_cache can go as a concept.

We use it for two cases:

a) #post_render_cache => JS, we can attach the right JS directly via drupalSettings.
b) #post_render_cache for placeholders, we can directly use the new placeholdering API with better DX and way better extensibility for rendering with different methods.

And even the is-active class can be simulated with a placeholder:

x-drupal-placeholder-is-active:[hash-of-path]

$build['#cache']['placeholders']['x-drupal-placeholder-is-active-[hash of path]'] = [
  '#pre_render_cache' => [
    'activeClassService.addIsActiveOrNot' => [ 'path' => 'path-of-link' ],
  ],
  '#cache' => [
    'contexts' => ['url'],
  ],
];

And then just display the 'is-active' if the path matches and '' if not.

--

More detailed feedback later.

Fabianx’s picture

#39:

  1. OK, was just proof-of-concept.
  2. Yes, unlikely someone has two body parts, can use the $count argument to explode to force. Good point.
  3. That is not necessarily true, a middleware might have changed the content. e.g. an access aware middleware that removes chunks with a certain data-access-filter attribute.
  4. With the javascript implementation I had before, I had gotten a fatal, but yes that is wrong, but just proof-of-concept code, too.
  5. PoC: Yes, ajax framework takes care of that.
  6. PoC: Yes, ajax framework takes care of that.
  7. This is a different case, e.g. the information to placeholder has bubbled up. Therefore we have a temporary static cache for placeholders whose final rendered data we know, else we would need to re-do all the work again and page time would be TTFB = 3s, Deliver = 3s which is kinda non-sensical. With the split to RenderCache service we need to find a new place for this data.
  8. Because the code below depends on $pre_bubbling_elements and I did not want to change more than necessary for the re-roll. Can be done differently. Also minimally need #pre_render_cache property, too.
  9. See 8.
  10. The elements data is already on the stack, but that is wrong as cacheSet can change it, but we still need to apply the data on the stack to the element before doing so, so that bubbled data is available.
  11. Agree, good thing.
  12. Proof-of-concept, moot with render strategies
  13. Cache system is responsible for placeholdering, because e.g. a bubbled placeholder information needs to be stored in the cache item / cache redirect item to support the b) use cases.
  14. This is actually pretty simple. #pre_render_cache is just for creating the #pre_render structure needed to run the alter hook, for BC its called at the same point. Was the easiest implementation. Other placeholder users don't need this.
  15. Yes, agree.
Wim Leers’s picture

Looks like we want to move forward with removing #post_render_cache and adding "placeholders" instead. For now, it looks like that will be significantly easier to understand. So, pushing forward with that in two child issues, since they are the first immediate blockers.

  1. Remove the only use of #post_render_cache that cannot be expressed as a "placeholder": #2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback
  2. Actually introduce "placeholders", and remove #post_render_cache: #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache

The first issue already has a patch, now working on a patch for the second.

Wim Leers’s picture

Wim Leers’s picture

Title: Implement BigPipe streaming for core [Render Cache Placeholders] » BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
Issue summary: View changes

Better issue title. Updated IS' remaining tasks to reflect #46.

Wim Leers’s picture

Issue summary: View changes

IS update: s/frequency/cardinality/

plach’s picture

Any feedback on #38?

Wim Leers’s picture

#38: I think the optimal answer depends on your site. But, we want to work out of the box, of course, so let's look at that.

I'd be roughly tempted to say "well, 99% of users have JS enabled, and we already assume JS is available for authenticated users (but we don't assume it for critical features like this one), and JS is simply a necessity for accessibility". But, I also believe that the page should still be completely usable even if JS is disabled.

Which then would indeed mean that the first page load would need to use the current render strategy ("Single Flush"). But I think that is fine: before one is authenticated, one needs to log in first anyway. So, this detection/setting of a cookie SHOULD happen during logging in (I'm tempted to write "MUST"). I think that should be perfectly plausible.

Also note that nobody is proposing to do this for anonymous users out of the box; anonymous users get *zero* JS out of the box on D8, and benefit from the Internal Page Cache already anyway. So we have absolutely no reason to enable BigPipe for anonymous users. Only very, very, very advanced Drupal 8 sites that build dynamic, session-dependent content would want to use BigPipe for anonymous users.

And then there really is no degradation, nor a performance hit:

  1. you want to log in, which means you're still anonymous, which means you benefit from the Internal Page Cache
  2. you log in, during the logging in we figure out that you have JS enabled, set a cookie,
  3. which means we now know that it is safe or unsafe to opt you in to the BigPipe render strategy!
plach’s picture

Sounds good and what about the HTML architecture? Any caveat or it will be transparent to themers which rendering strategy is used?

Wim Leers’s picture

Yes, it will be transparent.

Crell’s picture

I would like to note for the record that I am in tears with how happy I am to see #post_render_cache going away in favor of explicit placeholder support. Wim, we may end up seeing eye to eye on request/rendering yet. :-)

However, Wim brings up a good point. If we want to be able to vary the render strategy by user (or some other criteria beyond just "site"), that means we need to have a more robust mechanism for swapping that than simply "well swap the service with a compiler pass". Basically we need something analogous to ChainRouter or BreadcrumbManager, which was the original intent with the HtmlFragment/HtmlPage architecture. Making that its own service (with the same interface as HtmlRenderer, just like other similar cases) means that we can then support potentially arbitrary logic for when to use what rendering mode. It is definitely a per-site decision that should be made from code, though, no UI.

Wim Leers’s picture

#54: :) /me gives Crell a hug.

Agreed on the principle.

About six months ago, I thought that PageDisplayVariant + RenderEvents::SELECT_PAGE_DISPLAY_VARIANT could end up being the sensible place for that, but now that feels wrong; this feel like a different level. Page Display about what is responsible for generating the render array representing the entire page. This sits at a lower (or higher, depending on your POV) level: once you have that render array, how do you efficiently deliver it?

In that light, I think RenderEvents::SELECT_RENDER_STRATEGY would make most sense. Just like we default to SimplePageVariant for the page display selection event, this could default to the "Single Flush" (i.e. HEAD's) render strategy. And implement an event subscriber to select a render strategy, just like for selecting the page display variant.

Only rough thoughts for now, but, what do you think, Crell?

mesch’s picture

This is a tremendous initiative; placeholders are a great idea with potential for significant performance gains.

I'd just like to add another voice to making sure this can work without JS enabled. Providing a means to select which render mechanism to use would be ideal.

Crell’s picture

My inclination is to use a chained service, not an event, as it has lower overhead most of the time.

Thinking aloud: The main difference between events vs tagged services is the performance profile. Actually there's 3 options:

1) Tagged services

2) Tagged services with lazy loading

3) Events

Tagged services have the lowest runtime cost. There is almost no overhead beyond a loop and checking for a "did not handle" case if applicable. The downside is that all services are always instantiated by the container, so if you have a lot of them and the first one responds 90% of the time that MAY be unnecessary service instantiation overhead. (Or little, depending on what dependencies those services have.)

Events are the opposite; they don't instantiate a subscriber until it's about to be called, so if most of them will typically be skipped then that saves a lot of instantiation. (This is only true if you're only using subscribers in a container-aware event dispatcher. However, Drupal is doing so for exactly this reason.) On the other hand, there's considerably more overhead for firing an event as the dispatcher has to do its thing for each listener. That overhead is lower now that we wrote our own dispatcher, but it's still higher than tagged services. I haven't benchmarked it to see how big a difference it is.

Tagged lazy services are the middle ground. No loading of services we don't use and a smaller overhead than events, but at the cost of a container-aware service that therefore hinders testing. It also doesn't do a great job of handling resolution of which service will handle the task, as it can only do static or pre-compiled simple mapping or else load each service one at a time and call an applies() method.

Tagged services are also slightly cleaner in that they have fewer implicit assumptions, so they're easier for developers to reconfigure if they need something funky on a particular site. Event listeners make it very very tempting to just inline logic into the event class itself, which for anything non-trivial you shouldn't do as it makes the code less reusable or testable.

(I should blog the above at some point.)

So the question is, what's the usage profile of this extension point? How many renderers do we expect a reasonable site to have, and will one greatly overwhelm all others? Remembering that the renderer is already being called from a View listener, so none of this will be loaded for non-HTML responses anyway.

My guess is we're looking at a grand total of less than a half dozen renderers even existing, and no more than 2 on any given site (all-at-once for anon, "something else" for authenticated). Given that, I would expect that a simple tagged service with chaining and potentially an applies($request) call would be sufficient, and anything more involved than that would be over-engineering.

Discuss. :-)

Wim Leers’s picture

(I should blog the above at some point.)

Yes!

Before going in more detail, can you please take a look at \Drupal\block\EventSubscriber\BlockPageDisplayVariantSubscriber and \Drupal\Core\Render\MainContent\HtmlRenderer::prepare(), which is what dispatches the event. (Generally speaking, just grep for RenderEvents::SELECT_PAGE_DISPLAY_VARIANT.) One note there: in events, you have stopPropagation(), which can overrule everything else basically. I think that's a valuable feature.
Once you've looked at that, apply your explanation from above. Then what do you conclude? Should that also be using tagged services instead? If not, then why does it make sense to have these related/analogous/similar things be implemented so very differently?

So, in short: my answer/thinking is: both should use the same approach. Which approach that should be, I'm not sure.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
42.35 KB

Re-roll, with an added hack of storing placeholder data on the renderer in a public property (brrr). Don't do that at home, kids!

Needs review just for bot-check, then CNW.

--

Needed that re-roll to more easily port code over for the #prc => placeholders issue.


To the #57 / #58:

I am for tagged services or lazy tagged services, because it is chain of responsibility and a strategy can determine to just replace _some_ placeholders and leave the other placeholders to a different strategy coming afterwards. Stopping an event is likely problematic as the last strategy is always single flush to render everything not yet rendered.

e.g. if the data is already present or cached there is no need to BigPipe it anymore, but we would still want to use ESI tags for all placeholdered data (as we want to cache in Varnish instead). Hence my proposal to add some kind of meta data to placeholders so that strategies can be informed better of what they deal with ...

Status: Needs review » Needs work

The last submitted patch, 59: bigpipe_for_auth_users-2469431-59.patch, failed testing.

Fabianx’s picture

I decided to fix test failures and add yet another patch in preparation of converting #post_render_cache to #pre_render_cache.

In this patch:

- Fix render tests for addCacheableDependency
- Harden placeholder ID creation (no more need for #cache keys to be set)
- Get cache also if only #pre_render_cache is set. That does not make sense but because the placeholder logic in this implementation (to be changed) lives in the cache layer, it is necessary.

This makes the conversion from an API standpoint as easy as:

             $output['comment_form'] = array(
-              '#post_render_cache' => array(
-                $callback => array(
-                  $context,
-                ),
+              '#pre_render_cache' => array(
+                 $callback => $context,
               ),
-              '#markup' => $placeholder,
+              '#cache_placeholder' => TRUE,
             );

And the caller like:

     );
     $comment = $this->entityManager->getStorage('comment')->create($values);
-    $form = $this->entityFormBuilder->getForm($comment);
-    $markup = $this->renderer->render($form);
-
-    $callback = 'comment.post_render_cache:renderForm';
-    $placeholder = $this->generatePlaceholder($callback, $context);
-    $element['#markup'] = str_replace($placeholder, $markup, $element['#markup']);
-    $element = $this->renderer->mergeBubbleableMetadata($element, $form);
+    $element['form'] = $this->entityFormBuilder->getForm($comment);
 
     return $element;

So not too much that needs to be changed here and DX gets better ...

In theory even $element += $this->entityFormBuilder->getForm($comment); could be used if a child key form is not preferred (that should not matter in practice though).

Fabianx’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
21.66 KB

The patch for #61 - testbot only.

Fabianx’s picture

In this patch (this is all supplementary work for the conversion of #post_render_cache to #pre_render_cache + #cache_placeholder):

- Moved X-Drupal-Accel header sending to before content is send
- Added very basic support for render strategies as the 'messages' block does not play nicely with the session handler in the stage where BigPipe runs and drupal_set_message() sets the page_cache kill switch anyway.

Note:

The race that drupal_set_message() can show a message after a 302 when the message was for example set by a POST - is already pre-existing in HEAD.

We need a bug issue for that ( if one is not already existing ) ...

Fabianx’s picture

Crell’s picture

In response to #58:

I looked at the page variant logic Wim indicated, and he walked me through it at the DrupalCon sprint. We both agreed that the current setup for that is very wrong. :-) (Event AND plugin???) Applying the logic above to that case:

- We will realistically have more than one variant active on a given site. (Think Blocks and Panels, each handling different URIs.)
- We will never *use* multiple on the same request.
- There will be some that are active but rarely if ever used (eg, the core default naked format).
- Thus, we should use tagged services with lazy-load.

I agree with Wim that we should use the same approach here as well. That is, #57 is still the case: We should use tagged services with an applies() method, and a chain-of-responsibility fronting object that is container aware to lazy load. That is, almost exactly the same thing as Breadcrumbs do right now.

We can switch the variants around later in another issue, but that's how we should proceed here.

Wim Leers’s picture

#65: Can you open an issue to discuss changing page display variant selection? That will be an API change, but will affect only a few contrib modules at most. We should talk to tim.plunkett and/or Berdir about that, and get their thoughts on changing that, since they're working on the D8 port of Page Manager.

Wim Leers’s picture

Issue summary: View changes

Updating next steps tentatively — it'll be mostly up to @Fabianx to determine the final order, but this is roughly what still needs to happen. #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache is getting close.

Wim Leers’s picture

Issue summary: View changes

Talked to @Fabianx. He agreed with the next steps I added in #67.

Opened a new child issue, for auto-placeholdering: #2499157: [meta] Auto-placeholdering. That is the next step after #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Ok, so #2349011: Support placeholder render strategies so contrib can support BigPipe, ESI… was actually meant to be step 3: render strategies.

Wim Leers’s picture

Fabianx’s picture

Btw. Great news for Varnish + BigPipe:

While streaming content is very incompatible with Varnish in general, it can work easily with using ESI, because Varnish actually streams responses that contain ESI tags, so all that needs to be taken care of is that the same placeholders as for BigPipe are used, but then rendered via an uncacheable ESI request instead of streamed as part of the initial response. Or rather in cases (fast-cgi) where the response can continue to be processed after all data has been send, the data could be micro-cached and the ESI requests would have a lock on that micro-cached data then return it once it is ready.

TL;DR: There are several possibilities to make BigPipe work with Varnish as well :). (I also have some other ideas if the above does not work out.)

#2349011: Support placeholder render strategies so contrib can support BigPipe, ESI… has a patch now, will add a new patch here shortly.

Fabianx’s picture

This one will fail, because it activates BigPipe by default, which obviously tests will not expect. Lets see how hard.

This patch is based on the render strategies: #2349011: Support placeholder render strategies so contrib can support BigPipe, ESI…

Implementing BigPipe is now really really straightforward (thanks to all the pre-work. Especially #2407195: Move attachment processing to services and per-type response subclasses made the ajax processing a pure joy. (Thanks Crell for insisting and thanks Wim for fixing.)

In this patch

  • - Added render_strategy.big_pipe service
  • - Added big_pipe service
  • - Added proper BigPipeResponse that gets the big_pipe service injected on construction time, so remains a pure value object.
  • - BigPipe now works without any inline JS in preparation for CSP in core
  • - Instead a big_pipe.js script is listening for new elements (@todo could filter to just
    )
  • - BigPipe checks for new elements every 100ms until 10s have passed or the stop signal has been received.
  • - This also means that any elements ready within the first 100ms are processed directly, before the first attachBehaviors() is send.
  • - If there are later placeholders, then the normal Drupal.attachBehaviors() event is used to process those, too once the document.ready() fires. (e.g. after 20s)
  • - A pre-defined white list of per-request unique placeholders ensures that no-one can use this mechanism to introduce a XSS issue
  • - Processing a rendered AjaxResponse client side was really simple now thanks to [#2484581].

Remaining work

- Clean up the Javascript (probably need nod_)

- Decide where the BigPipe code lives

My proposal is to: - Move the big pipe render strategy service to its own module. That way big_pipe can be enabled / disabled on demand. - Leave the default big_pipe service and interfaces in core.

- ajaxPageState is not taken into account

There is one blocker left, which can be follow-up or worked on before, because it seems all ajaxPageState is broken right now anyway: Proposal: See -ajax-page-state-proposal.txt (which is also included in main patch) The idea is to move the ajax page state to the AttachmentsResponseInterface, where it belongs. Then the BigPipe service (that could then take a Response) and the Ajax and Html Response Processors could take the Response from there if already set and also update it again after the settings processing has been finished. As that seems a little bigger, I am all for splitting that part off to another issue: "Re-work ajax_page_state to properly work with Responses." --- Unassigning for now.
Fabianx’s picture

Status: Needs review » Needs work

The last submitted patch, 74: bigpipe_for_auth_users-2469431-74.patch, failed testing.

Wim Leers’s picture

AWESOME! :) :)

Three initial observations, of things that surprised me:

  1. AFAICT this does not yet deal with detecting JS support. We should not break for auth users that don't have JS support, so non-JS users should be opted out of BigPipe automatically.
  2. Why have this "check every 100 ms"/"stop checking after 10 seconds" thing at all? Why not just wait until the HTTP response has finished streaming?
  3. +++ b/core/core.libraries.yml
    @@ -121,6 +121,15 @@ drupal.batch:
    +  drupalSettings:
    +    bigPipePlaceholders: []
    
    +++ b/core/misc/big_pipe.js
    @@ -0,0 +1,84 @@
    +      // Ignore any placeholders that are not in the known placeholder list.
    +      // This is used to avoid someone trying to XSS the site via the
    +      // placeholdering mechanism.
    +      if (typeof(drupalSettings.bigPipePlaceholders[placeholder]) !== 'undefined') {
    

    So I found why you have the placeholders in drupalSettings, but I don't understand the reasoning behind this XSS potential. Is it just in case a MITM hijacks the response and streams their own overridden placeholder replacements?

    This is equally problematic for regular (SingleFlush) responses, where a MITM could replace parts of the response.

Fabianx’s picture

#77:

1. Indeed that is missing :):

- @todo Add detection of Javascript
- @todo Add checking to only do this for auth users.

2. Because we want to render placeholders as soon as they become available, which is an important part of ensuring the user has a quick response. e.g. the sidebar should not have to wait for the very slow ads ...

3. The XSS potential is non-existing right now, but once CSP is in, the attack vector of <script> will be gone completely, BUT Drupal.ajax commands can execute arbitrary jQuery commands at the moment. So if an attacker puts in <script type="application/json" data-drupal-ajax-processor="big_pipe">{some ajax commands we don't want}</script> they could theoretically execute ajax commands that crack the site.

Yes, this only works once they have circumvented XSS as [script] is still protected, but we don't need to introduce a CSP-exploit-possibility in Core.
(I am a step ahead here; also I need to show its possible _without inline JS_ for various reasons ;) ...)

And we can't use inline JS as we can't make the output trusted. So protecting the streamed response data with unique tokens is the best way I could come up with.

Because you can't guess the unique token and because the response is uncacheable, you can't fake your own JSON data that is processed via the big_pipe ajax processor.

Of course we could also use totally random IDs for BigPipe and maybe take ['render_strategy_options']['#attributes'] into account ... (just food for thought).

dawehner’s picture

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -124,18 +134,38 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
+
+    if (!empty($content['#attached']['big_pipe_placeholders'])) {
+      $response = new BigPipeResponse('', 200, [
+        'Content-Type' => 'text/html; charset=UTF-8',

I'm curious whether big pipe could live outside of the main HtmlRenderer. Theoretically you could wrap the existing service and convert to a BigPipeResponse.

Fabianx’s picture

#79: Yes, likely we can use a very early FilterResponseEvent and do:


$response = $event->getResponse();

if ($response implements HtmlResponseInterface) {
  $attachments = $response->getAttachments();

  if (!empty($attachments['big_pipe_placeholders'])) {
    $new_response = new BigPipeResponse();

    // ... same code as in the patch ...

    $new_response->headers = clone $response->headers;
    $new_response->setStatusCode($response->getStatusCode());
    $new_response->setContent($response->getContent());
    $new_response->setCacheableMetadata($response->getCacheableMetadata());
    $new_response->setAttachments($attachments);

    $event->setResponse($new_response);
  }
}

That should likely work and isolate it completely into a module then ...

Fabianx’s picture

Just a quick patch to improve front-end performance by using a container for the placeholders.

Fabianx’s picture

Fabianx’s picture

Task list here:

  1. Move all to a big_pipe module
  2. Make it work again
  3. Ensure to check for authenticated users
  4. Detect if the JS cookie is present
  5. Add tests
  6. Fix ajaxPageState
Wim Leers’s picture

+++ b/core/lib/Drupal/Core/Render/BigPipeResponse.php
--- a/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -166,9 +167,27 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
+    if (!empty($content['#attached']['big_pipe_placeholders'])) {
+      $response = new BigPipeResponse('', 200, [
+        'Content-Type' => 'text/html; charset=UTF-8',
+      ]);
...
+    else {
+      $response = new HtmlResponse($content, 200, [
+        'Content-Type' => 'text/html; charset=UTF-8',
+      ]);
+    }

This is still modifying HtmlRenderer? :( It shouldn't. It should be purely a placeholder strategy…

If this needs a different response class (a subclass of HtmlResponse), then you can just have a subscriber running before the HtmlResponseSubscriber that takes a HtmlResponse object and maps it to a BigPipeResponse object.

Fabianx’s picture

Moved to big_pipe module and made it work again.

- Main change needed is to ensure attachments can include data needed for a later subscriber; unfortunately the re-factor in #2477223: Refactor _drupal_add_html_head, drupal_get_html_head, _drupal_add_html_head_link into the attachments processor, remove from common.inc. does not solve that.
- Also needed to clone the HtmlResponse

Still based on top of #2349011: Support placeholder render strategies so contrib can support BigPipe, ESI….

Fabianx’s picture

#84 is invalid, because that was based on #83, which is just a re-roll, not the fixed version as in #85.

Fabianx’s picture

Just a quick interdiff and based on render strategies patch.

Implemented:

- A really nice has_js check that does not need JS to work :p and is stored in the current user's session.
- Blacklisted some placeholders
- Added authenticated and permission checks


Next step:

- Figure out a way for AjaxPageState to work; currently this trashes the getJSAssets cache and makes page slow
- Cleanup, cleanup, cleanup

Possibly might need to change placeholder strategies to take a Response object in addition to the placeholders array.

Status: Needs review » Needs work

The last submitted patch, 85: bigpipe_for_auth_users-2469431-84.patch, failed testing.

nod_’s picture

Shouldn't the JS use jquery.once instead of the custom big-pipe-processed data attribute?

The inline JS would get in the way of enabling CSP too.

Wim Leers’s picture

#89: Where do you see inline JS? It's only JSON.


  1. +++ b/core/modules/big_pipe/big_pipe.module
    @@ -1,2 +1,56 @@
    +function big_pipe_form_after_build($form, FormStateInterface $form_state) {
    

    Wow… this is very clever :D

  2. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -19,17 +20,60 @@
    +    // @todo Move to a ResponsePolicy instead.
    

    But those policies are only for caching. This is not for caching.

    Why not just make it configurable? (i.e. the BigPipe module can ship with a Config .yml file that only enables it for authenticated users by default.)

  3. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -19,17 +20,60 @@
    +    // @todo Add user.permissions cache context.
    +    if (!$this->currentUser->hasPermission('Use BigPipe placeholder strategy')) {
    +      return $return;
    +    }
    

    What's the use case for such a permission?

    (And if we do have a use case: permissions always have lowercase verbs.)

  4. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -19,17 +20,60 @@
    +        // The messages element is not able to update the session when BigPipe runs.
    

    Let's document why, because this IMO requires a fix in core.

  5. Possibly might need to change placeholder strategies to take a Response object in addition to the placeholders array.

    Why?

  6. Figure out a way for AjaxPageState to work; currently this trashes the getJSAssets cache and makes page slow

    I could take this on if you want. Curious to see how this "trashes" that cache :)

Fabianx’s picture

#89: As Wim wrote, application/json is the same as we use for drupalSettings, so not sure where there is inline JS?
#90: Thanks for the review!

1. Thanks! Some credit goes to nod_ in the original has_js removal issue though ;). Even though I used <noscript> in a different way.

2. Indeed, should we have a placeholder policy or something like that, or just put it in a method and then someone can subclass it to there needs? A config.yml sounds also nice ...

3. Hmm, I thought of being able to black-list some roles, like e.g. administrator. But probably not needed, yes. As administrator has all permissions anyway. Will just remove for now.

4. Agree, I document those as @todo's for now, so we can see what is needed to fix in core for that.

5. Well, placeholder strategies can react differently based on e.g. permissions or user.roles or session, so they very likely need to be able to specify cacheable metadata. There is two ways: Provide a cacheable_metadata object they can add to or provide the Response directly. Mis-use is unlikely as someone could as well write a full EventSubcriber for that case, so its not that passing that in would hurt much ...

6. I saw a Cache::set every page request when BigPipe was active in {getCss/getJs}Assets. Easy to xhprof with the latest attached patch on top of Placeholder Strategies.

nod_’s picture

My bad about the inline JS, got confused. Carry on :)

yched’s picture

big_pipe_form_after_build() : that seems tighly coupled to "login via the default login form", aren't there other ways to open a session ?

Wim Leers’s picture

#91:

  1. AFAICT no cacheability metadata is necessary here, because this is solely about delivery of HTML. When using BigPipe, we still want SmartCache to work. So, HtmlResponseBigPipeSubscriber should run after SmartCacheSubscriber::onResponse() (which already is the case).

    Therefore, the only caching we see here is:

    • The caching of the underlying HtmlResponse by SmartCache. This patch does not change anything about that; it's 100% layered on top.
    • The caching of the placeholders that are being rendered and delivered by BigPipe, which is done by calling Renderer::renderPlaceholders(). That method performs render caching just like always.

    In other words: BigPipe is layered transparently on top of all existing infrastructure, and merely delivers the HTML differently. It does not affect the cacheability of anything in any way AFAICT.

    Or what am I missing?


#93:

big_pipe_form_after_build() : that seems tighly coupled to "login via the default login form", aren't there other ways to open a session ?

Great point.

Can't we add something to the html.html.twig template, or perhaps use hook_page_attachments() to add the <noscript> stuff to <head>? (Just trying to think out of the box here.)

Fabianx’s picture

#94:

Discussed some more in IRC:

I totally agree that if there really is complicated policy decisions for a 0.01% case such should just use its own EventSubscriber to supplement things, so no need to complicate the API for all consumers.

Also it is a good idea to say that at onResponse level all caching (except page_cache for anonymous users) is basically done. That is a good line to draw.

#93:

Yes, however the only possibility would be to add that to all forms, which feels excessive. If there is some kind of SSO, then it is not guaranteed that even any page is executed by Drupal, but it could all be cached until the user comes to an uncached page. While we could in addition set a cookie by JS, that feels excessive and I think it would be better for such a third-party login to set the cookie themselves. Most D7 third-party logins re-used parts of user_login_form though, so that should still work.

The problem with not adding it to a form that is POST'ed is that then JS needs to be executed for the anonymous user, which we don't want.

So as authenticated responses vary by cookie / session anyway, what about adding a JS to the page via hook_page_attachments_alter() that does an ajax callback (dynamically checked for via drupalSettings, which are set via hook_javascript_settings_alter()) to set the SESSION variable - in case the $_SESSION variable is not set at all. (the 5% case).

If JS is on, then the AJAX request succeeds and sets it so the AJAX is never called again, if its off the AJAX request does not work in the first place, so nothing lost in adding that JS for that user.

That feels to me the cleanest fallback (and is a technique I frequently use with Akamai).

yched’s picture

Not sure I follow the "If there is some kind of SSO, then it is not guaranteed that even any page is executed by Drupal" reasoning, but the proposed approach with hook_page_attachments_alter() / hook_javascript_settings_alter() sounds nice ?

That even seems like a generic solution that might be worth moving higher up than just $_SESSION['big_pipe_has_js'] in big_pipe.module ? If some other module needs to know on the server side whether JS is enabled, then it would be sad if it had to duplicate that same logic (several ajax requests, duplicate info bloating $_SESSION...)

In fact, that's a drawback of that approach : if not shareable and everyone does the same, that doesn't scale.

Fabianx’s picture

#96: So we need a HasJS service to coordinate that? That works for me.

Where should it live? system.module? user.module? new custom module?

yched’s picture

I'd suggest system.module, but curious to know what JS maintainers think :-)

yched’s picture

Or even : if that moves to the "system", then could it be baked directly into our JS processing without messing with hook_page_attachments_alter() / hook_javascript_settings_alter() in a module ?

Like, restore an official "has_js" functionnality baked in the system, but without the cookie (which was the real issue, right ?).

I guess that would be a call for our JS guys - to be on the safe side, maybe it's best to keep it in system.module alter hooks for now ?

Fabianx’s picture

Discussed with nod_:

He is not keen to bring back server capabilities, so the deal is:

- Assume 90-95% of users have JS on
- Assume that a user can turn off JS at any time

Solution:

- Assume JavaScript is on
- Add <meta http-equiv refresh=""> inside <noscript> tags inside the html_head to forward to /bigpipe/js-off?destination=<orig url>
- That user is then blacklisted for the remainder of their session and apart from 2 redirects, it is 100% transparent and gracefully degrades the UX

While that could still be made generic, many other use cases can probably use tags in a different way.

yched’s picture

Yeah, <meta http-equiv refresh=""> inside <noscript> is also how we handle non-ajax progression on batch pages in D8 (not in D7 since noscript is allowed inside the head section only in HTML5), so that makes sense.

Not sure either it's worth trying to shape that into a generic feature - at least that doesn't have to be in this issue here.

Also, agreed that (re-)introducing a formal ability to differenciate JS and no-JS on the server side could open a can of worms. E.g. we'd need a corresponding 'has_js' cache context to differenciate cached markup. Maybe it's best if we can avoid going there.

So, +1 to #100 :-)

Fabianx’s picture

Plain re-roll on top of latest core (with render strategies fixed) for visibility, no changes.

Fabianx’s picture

MAJOR CHANGE

Meet BigPipe's little brother/sister HalfPipe! BigPipe without JS!

HalfPipe is a render mode of BigPipe to allow to send content in chunked ways.

e.g. send until the first placeholder, calculate the placeholder, then send the next chunk.

With the Exception of JS settings in the header (then BigPipe / HalfPipe bails out of processing), HalfPipe is 100% transparent to the user and just means we deliver the page faster.

Together with smart cache a page can be served in 35-40 ms - even if the content download takes 2.9 s (!) (This uses a separate patch for EntityViewBuilder to use #lazy_builders on the front page view).

Obviously neither HalfPipe nor BigPipe are able to change headers anymore, but in the case of HalfPipe any newly attached libraries (of the placeholders) are accumulated, then the 'styles' of the header (if any) are send and then the 'scripts' (if any) are send (header scripts).

While neither the styles nor header scripts are actually in the header, they are still before the bottom scripts, which could be enough for many cases.

And then last but not least the combined JS settings + JS bottom scripts are send (of the whole page response).

So apart from maybe slightly different attachments / aggregates (and this can be fixed by statically adding them always via e.g. hook_page_attachments_alter(), this is 100% transparent to users and search machines.

---

And then any further BigPipe placeholders are processed. BigPipe has the big advantage that the whole page is present already and the main JS is already run by "faking" the DomContentLoaded event. (@todo support domready library, too and not only Drupal.attachBehaviors).

--

But in combination those two rock!


The attached patch is very very rough at the edges and needs some good thought, but it all works :).

[ Turn JS off, login, Turn JS back on, enjoy! ]

neclimdul’s picture

Last patch seems break actions on controllers that are forms. node edit, config forms, etc.

Wim Leers’s picture

Wim Leers’s picture

Fabianx’s picture

Short update on this. There is mainly three technical problems, one UX problem and one cleanup remaining:

I think we should try to get this in 8.0.x - after seeing the Dries Note. So I am gonna summarize the things remaining.

What we have solved so far, but not fully implemented is:

- JS less BigPipe via SmallPipe (chunked sending of content) [ needs cleanup ]
- Assume JS by default, but if disabled, black-list the user session by setting a cookie via a meta http-equiv refresh redirect in noscript tags. [ needs implementation ]
- Opting out of SmallPipe / BigPipe via #render_placeholder_options [ needs discussions / cleanup ]
- Fix the JS and remove the drupalSettings dependency
- Ensure SmallPipe never runs when JS settings are in the header (BigPipe can still work fine). [ that is an okay limitation for now ]
- Automated testing (web tests)

Technical problems

  • 1. BigPipe needs a way to take over the sendContent method of HtmlResponse
  • 2. BigPipe needs to work with Varnish
  • 3. BigPipe needs to correctly handle assets

1. BigPipe needs a way to take over the sendContent method.

Discussed this with Crell yesterday and there is a "EmitterInterface" in PSR-7, which we can copy.

Then BigPipe can do something like:

$response->setEmitter($big_pipe);

and thats it. SOLVED! - Needs split into child issue and implementation!

2. BigPipe needs to work with Varnish.

It turned out this is very easily possible:

It needs one line in vcl_fetch():

sub vcl_fetch {
+  set beresp.do_stream = true;

The only drawback to that is that it is not compatible with ESI (by design), so we need to distinguish, the proposal is to use the 'Surrogate-Control' header:

Drupal sends:

Surrogate-Control: content="BigPipe/1.0"

Varnish does:

sub vcl_fetch {
  if (beresp.Surrogate-Control ~ "BigPipe/1.0") {
    set beresp.do_stream = true;
  }
  if (beresp.Surrogate-Control ~ "ESI/1.0") {
    set beresp.do_esi = true;
  }

SOLVED! - Needs implementation.

3. BigPipe needs to correctly handle assets.

This one is tricky, but overall just a problem if a theme overrides things directly.

Either !important or a more specific selector can be a friend or hook_library_info_alter().

Together with libraries-extend and libraries-override that _will_ allow to have SmallPipe + BigPipe + AJAX working out of the box.

It is also not a new problem as it is the same for any AJAX request that has assets.

Another concern is to create too many assets. I think the solution for that is to optimize the code / placeholders and just add the assets statically to the BigPipe placeholders itself or via something like hook_page_attachments_alter().

We need to make that as simple as possible, but that is probably the remaining challenge here.

PARTIALLY SOLVED - There is not much we can do in here to make that better - it needs changes elsewhere in core.

User Experience

The biggest things against enabling SmallPipe / BigPipe by default for all things is three issues:

a) The page can become very jumpy in where content appears. It is best to target some parts of the page for BigPipe'ing to get the best effect. This has improved a lot already by ordering by order-of-appearance in the send DOM.

Especially with SmallPipe that is way way less of a concern, but some users might want to block the page rendering to avoid flash-of-unstyled-content.

b) What to BigPipe / what not to BigPipe and how to distinguish - which applies to Placeholder strategies in general:

e.g. It never makes sense to BigPipe the CSRF token or the form action placeholder, but makes a lot of sense to ESI the CSRF token, but never the form action placeholder.

skyredwang’s picture

Obviously, I am super late to this issue. But, I was wondering has any effort being done to consider using native Web Components instead of using JS/Ajax approach? With Web Components, Drupal would output the "expensive" parts as shadow doms, waiting the client's browser to natively "HTML Imports" them. For compatibility support, we can easily borrow the best practices libraries like Polyfills to ensure the "expensive" parts will load if their browsers don't have native support.

Fabianx’s picture

Short update on #107:

After speaking with lots of people during DrupalCon:

- A very simple way is to use a theme template + suggestions (based on the lazy builder or cache parameters) to allow styling BigPipe placeholders on the theme level and hence great a good effect.

Overall the following things are missing that are outside the module space itself:

  • 1. EmitterInterface + EmitterTrait - straight port from PSR-7, allows to overwrite send() if $this->emitter is set: Just needs to be done - issue created, etc.
  • 2. AjaxPage state handling. While the current patch does work correctly, it sets some things from protected to public, which needs discussion and a proper solution.
  • 3. Unsupported types in #attached - How do we make this configurable. In the worst case we could patch in a special render_pipe_placeholders or streamed_placeholders for now into core - but does not feel too good - especially if its a module.
  • 4. #create_placeholder_options - I think we should just go with that name and use something like ['#create_placeholder_options']['streaming'] = FALSE for now and maybe also allow [...]['streaming'] = 'big_pipe' - not sure.

There is not too many ways to stream things overall.

Wim Leers’s picture

#2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags landed in the mean time, causing the patch to no longer apply. Rebased.

#108: that's exactly the direction I was thinking in also :), but none of that is in a mature state yet, and definitely not within Drupal. So I think it will make sense to evolve in that direction, but we shouldn't want to do that here; it'd expand the scope far too much.

Wim Leers’s picture

#109:

  1. Fabian filed #2577631: Allow HtmlResponse to use a flexible emitter for this.
  2. I think there may be a different way to do this, that doesn't need these changes.
  3. I think the solution lies in decorating HtmlResponseAttachmentsProcessor: BigPipeEnabledHtmlResponseAttachmentsProcessor, and we update HtmlResponseAttachmentsPRocessor (the original) to do the validation in a helper method. Then the decorating class can change that.
    We can always expose more APIs in 8.1, but taking it back is harder, so I'd prefer to go with such an approach first.
  4. I've been doing some thinking around that already. I think the key problem is for things that are NOT HTML versus HTML. For example: CSRF token, form token. > The placeholder that we put in those places is HTML, even though it's in places where you can't use HTML. So I think the key thing to do there is to *know* whether a placeholder is going to contain HTML or not. And I think that's the only thing we _really_ need to know. Something like #non_html_placeholder => TRUE. I think this point needs the most discussion.

I will work on points 2 and 3 tomorrow.

Wim Leers’s picture

BTW, some of the changes in this patch are actually a straight bugfix, which is being handled over at #2497115: ajax_page_state is not taken into account for normal GET requests, which already is RTBC.

Wim Leers’s picture

I was working to address #111.2.

But it turns out that we are sending the libraries that already were loaded in the initial page (the "skeleton") again with every placeholder. See below. Fabianx tells me that this used to work correctly, so we must've regressed somehow.

  <script type="application/json" data-drupal-selector="drupal-settings-json">{"path":{"baseUrl":"\/","scriptPath":null,"pathPrefix":"","currentPath":"user\/1","currentPathIsAdmin":false,"isFront":false,"currentLanguage":"en"},"pluralDelimiter":"\u0003","ajaxPageState":{"libraries":"bartik\/global-styling,big_pipe\/big_pipe,classy\/base,classy\/messages,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/normalize,quickedit\/quickedit,shortcut\/drupal.shortcut,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons","theme":"bartik","theme_token":null},"ajaxTrustedUrl":{"\/search\/node":true},"bigPipePlaceholders":{"drupal-render-placeholder-callbackdrupalblockblockviewbuilderlazybuilder-arguments0bartik-local-tasks1full2-token7daf9cccdrupal-render-placeholder-":true},"toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"WTNwGh7Oo4QFtk8WHEZpSoP25uCSTNCcux3S7_caJaM"},"bigPipeResponseMarker":1,"user":{"uid":"1","permissionsHash":"8ded07c12671cf92b43603f7d8478f135b94f019b8d38fb757a71e14d2ea2ce6"}}</script>
<script src="http://tres/sites/default/files/js/js_BKcMdIbOMdbTdLn9dkUq3KCJfIKKo2SvKoQ1AnB8D-g.js"></script>

<!--[if lte IE 9]>
<script src="http://tres/sites/default/files/js/js_VhqXmo4azheUjYC30rijnR_Dddo0WjWkF27k5gTL8S4.js"></script>
<![endif]-->
<script src="http://tres/sites/default/files/js/js_lcDBLUUzBrdhiSC3fM55qpKWqdfB0C9Lqme7t8rqkL8.js"></script>

  <div data-big-pipe-container="1">
    <script type="application/json" data-big-pipe-start="1"></script>
    <script type="application/json" data-big-pipe-placeholder="drupal-render-placeholder-callbackdrupalblockblockviewbuilderlazybuilder-arguments0bartik-local-tasks1full2-token7daf9cccdrupal-render-placeholder-" data-drupal-ajax-processor="big_pipe">
    [{"command":"settings","settings":{"bigPipeResponseMarker":1,"ajaxPageState":{"theme":"bartik","libraries":"bartik\/global-styling,big_pipe\/big_pipe,classy\/base,classy\/messages,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/normalize,quickedit\/quickedit,shortcut\/drupal.shortcut,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons"},"pluralDelimiter":"\u0003",…

Also, the HTML and AJAX response attachment processors use the request's AJAX page state (i.e. client-provided), but BigPipe uses the processed one, from hook_js_settings_alter(), i.e. after system_js_settings_build() and system_js_settings_alter() have run. BigPipe has to do this because the request's ajax page state variable is always empty on HTML GET requests, it is only set for AJAX requests.

Therefore, we need to look into unifying/formalizing AJAX page state handling, because that is crucial for BigPipe. But, given that this only affects APIs that are implemented by at most 10 people ever and therefore affect <0.01% of developers, I'm convinced this is fine to happen after beta 16 (today) and RC1 (next Wednesday). So let's focus our energy on things important for those two releases first, and then tackle this. I'm happy to take this on.

Wim Leers’s picture

Fabianx’s picture

#2577631: Allow HtmlResponse to use a flexible emitter is a must-have as neither cloning the response nor creating our own Response is a clean solution in the end - as discussed with Crell.

Also it sends very much the right signal - to be able to flexible stream responses, which is kinda a revived hype at the moment.

Also it is just PSR-7 back-porting overall and could well live in the future in Symfony upstream and be allowed for all Responses.

Wim Leers’s picture

Okay, but then we'll need to talk to a committer to get this done during RC. It's kinda sad that we first said we had all BigPipe blockers out of the way and now there's a new one.

See #2577631-5: Allow HtmlResponse to use a flexible emitter — tagged it rc target triage to get committer feedback.

larowlan’s picture

Now the RC is out - is this off the table?
Should we be splitting out the bits that aren't in the big pipe module into their own patch so that the module can live in contrib until the next point release?

catch’s picture

#115 is that really a hard-blocker or a nice-to-have though? i.e. does not having emitters introduce technical debt we can't just remove again when emitters lands (because we'd have to support the less-clean pattern beyond that point)? Or is it just a nicer API for what would be an ugly implementation detail?

The AJAX bits here look worth splitting out unless they're actually not relevant.

Wim Leers’s picture

#115 is that really a hard-blocker or a nice-to-have though? i.e. does not having emitters introduce technical debt we can't just remove again when emitters lands (because we'd have to support the less-clean pattern beyond that point)? Or is it just a nicer API for what would be an ugly implementation detail?

+1 — I basically asked the same question at #2577631-20: Allow HtmlResponse to use a flexible emitter. Let's put the answer to that on that issue, so that it's all centralized.


The AJAX bits here look worth splitting out unless they're actually not relevant.

I *think* it's just this patch that needs to be fixed. I'll need to dig into this patch to be certain though.

catch’s picture

@larowlan I think this is all 8.1.x now, except it looks like a potential API change with the AJAX changes that me might want/have to do during RC.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Working on the AJAX stuff.

Fabianx’s picture

catch’s picture

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
@@ -105,16 +105,19 @@ public function __construct(AssetResolverInterface $asset_resolver, ConfigFactor
     // @todo Convert to assertion once https://www.drupal.org/node/2408013 lands

This is adding an optional parameter to a method on an interface.

Which is legal in PHP, but not sure we really want to do that?

Wim Leers’s picture

FileSize
34.68 KB

To start: a rebased version of #110, with conflicts resolved. (#2497115: ajax_page_state is not taken into account for normal GET requests caused conflicts.)

Wim Leers’s picture

--- a/core/lib/Drupal/Core/Asset/LibraryDependencyResolver.php
+++ b/core/lib/Drupal/Core/Asset/LibraryDependencyResolver.php
@@ -94,7 +94,7 @@ public function getMinimalRepresentativeSubset(array $libraries) {
       }
     }
 
-    return $minimal;
+    return array_unique($minimal);
   }
 

I'd also like to see this moved to a separate issue. It's a plain bugfix. It doesn't belong here. Especially because we also need test coverage for this. So, opened #2594669: LibraryDependencyResolver::getMinimalRepresentativeSubset() accepts a set as the input but doesn't complain if it's not actually a set and filed a patch with test coverage. Turns out that this isn't actually a bug in LibraryDependencyResolver, but somewhere in this patch, because getMinimalRepresentativeSubset() only returns duplicates if what it is given is not a set, which is wrong.

I'll find and fix the code in this issue/patch that is not passing a set to getMinimalRepresentativeSubset, which should allow me to remove that change.

Wim Leers’s picture

FileSize
34.36 KB
3.28 KB

In #90.3, I remarked that the permission added here is kinda pointless, and Fabianx agreed in #91.3. He said he'd remove it, and he did, but the *.permissions.yml file is still there. Removing that. Slightly simpler patch :)

Wim Leers’s picture

FileSize
412 bytes

That was the wrong interdiff, clearly.

Wim Leers’s picture

FileSize
33.92 KB
975 bytes

Given #2594669: LibraryDependencyResolver::getMinimalRepresentativeSubset() accepts a set as the input but doesn't complain if it's not actually a set has an RTBC fix, the unnecessary change I pointed out in #125 can be removed here.

One less hunk again. Slightly simpler patch again.

Wim Leers’s picture

FileSize
34.06 KB
2.56 KB
569 bytes

#128 had the wrong interdiff. Attached interdiff-128.txt for that. Sorry.


#2576533: Rename SafeStringInterface to MarkupInterface and move related classes and #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder broke this again. Fixing.

Wim Leers’s picture

FileSize
34.59 KB
1004 bytes

While debugging, I noticed that bigpipe_js_settings_alter() would run before system_js_settings_alter().

bigpipe_js_settings_alter() should always run last. This fixes that.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.43 KB
7.81 KB

This patch is the key reason I'm working on this issue. It's to either fix #2578259: AttachmentsResponseProcessorInterface::processAttachments should take ajax_page_state as argument or show it to not be necessary. And it shows that #2578259 is not necessary.

It's perfectly possible to not use the response attachment processors directly, and to instead dispatch a KernelEvents::RESPONSE event on the response object instead. Which then results in the normal response handling. Which also means that if there are additional response event subscribers that e.g. extend or enhance the AJAX system, they will also apply to BigPipe now.

All AJAX page state-related changes in AjaxResponseAttachmentsProcessor and HtmlResponseAttachmentsProcessor are gone now. I haven't yet injected all the necessary services, because I first want a +1 on this direction.


In the mean time, I'm working on further ideas to make this even more understandable/less special/less confusing. Ideally, we wouldn't need to get the AJAX page state into BigPipe using hook_js_settings_alter() — it works reliably, but it still feels more hacky than I'd like. Perhaps it'll cause me to reconsider the direction in this patch, but at least this shows clearly that it is possible to implement BigPipe without API changes.

Wim Leers’s picture

Issue summary: View changes
Issue tags: +API addition
FileSize
32.44 KB
2.7 KB

The previous patches already made Renderer::renderPlaceholder() public. This removes the accompanying @todo and makes it official. This is a pure API addition.

IS updated accordingly. Note that this API addition is not essential; it could be added later during the D8 cycle. The contrib BigPipe module could duplicate this during 8.0.x, but ideally it'd be a public API at some point.

Fabianx’s picture

I like the new direction and I dislike it at the same time.

I still dislike that stateless services depend on global state (request_stack) even though it is completely unnecessary and state could be given as needed.

Overall I think it is okay to use sub requests or something like it (though we carefully need to assess performance).

It is obvious that using a new sub request will solve the problem of the ajax page state. It also makes other subscribers run which is a win.

And it keeps the original request unchanged.

And the other hunk (small pipe) that uses the html response processor can at least push a fake request on the request stack and still directly use the HtmlFragmentRenderer, because HtmlResponseSubscriber only runs for the master request.

The question that someone then could ask is:

- Why not go via a sub request in the first place and use a custom $request->data attribute and a controller handler (similar to Symfony's own fragment listener) to render the data?

I am not sure if that would be good or bad or how performant it would be, but that is the Symfony idea of ESI and rendering fragments of data.

The only difference would be that we would not use an URL (though we could still use an url like /_fragment/[hash] to consolidate), but push the data instead in a data attribute.

On the other hand lazy builders could be transformed into an URL as well, just the caching part of the array makes that a little difficult ...

Just some food for thought.

References:

- http://symfony.com/doc/current/book/http_cache.html#using-edge-side-incl... (especially _fragment portion)
- https://api.drupal.org/api/drupal/vendor!symfony!http-kernel!EventListen...

Wim Leers’s picture

I like the new direction and I dislike it at the same time.

That's exactly how I feel :)

I still dislike that stateless services depend on global state (request_stack) even though it is completely unnecessary and state could be given as needed.

Drupal is for building web sites. So we deal with requests and responses. And here, we have something (an AttachmentsResponseProcessorInterface) that receives a response and is meant to process it.

Is it then so strange/wrong for that to also look at the corresponding request to do the processing it wants to do? (And note that it may actually look at something else than just AJAX page state, which would make the changes proposed in #2578259 relatively pointless, even though it's without a doubt the most common case.)

I think not.

Don't we do the same in many other places?

- Why not go via a sub request in the first place and use a custom $request->data attribute and a controller handler (similar to Symfony's own fragment listener) to render the data?

I first did exactly that. But then I ran into some problems. IIRC the most notable problem was that doing a subrequest means doing routing etc. as well, even though that is utterly pointless here. And even semantically wrong in this case: a BigPipe response is essentially an aggregated response: it consists of a HtmlResponse plus multiple AjaxResponses combined in a BigPipeResponse. We don't need nor want routing for each of those AJAX responses, since they're all built from the result from a single controller. So: 1 request -> 1 BigPipe response (which consists of 1 HTML response + N AJAX responses).

I think the fact that we don't need routing nor controllers for the N AJAX responses is sufficient reason to not use subrequests. It makes sense to have multiple KernelEvents::RESPONSE events fired for one request -> route -> controller. Using subrequests requires to you have either a route to point to or at least a controller to point to (http://symfony.com/blog/new-in-symfony-2-2-the-new-fragment-sub-framework) — we have neither here.


Having answered this, this confirms my belief this is the right direction.

Wim Leers’s picture

FileSize
32.86 KB
8.86 KB

Here's a review round, and fixed all of my remarks myself. More such review rounds to follow, all of which should make the patch more understandable and ready.

  1. +++ b/core/modules/big_pipe/big_pipe.module
    @@ -0,0 +1,80 @@
    +  // This is tricky: We want Form API to default big_pipe_has_js to 1 in
    +  // case it gets not send. We also want to set the value of the HTML element
    +  // to 0 and add <noscript> tags.
    +  // So in case the user has JS disabled, the noscript is parsed and
    +  // big_pipe_has_js is send with '0', else it is not send at all and FAPI falls
    +  // back to the default value, which is '1'.
    

    Typos and inconsistencies here.

  2. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,127 @@
    + * HTML response subscriber to replace the Response with a BigPipe Response.
    

    s/Response/HtmlResponse/
    s/BigPipe Response/BigPipeResponse/

  3. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,127 @@
    +   * Processes placeholders for HTML responses.
    

    Wrong comment.

  4. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,127 @@
    +      // Remove any existing markers.
    +      $content = str_replace('<drupal-big-pipe-scripts-bottom-wrapper>', '', $content);
    

    I don't see how this could ever occur? Removed this line to avoid confusion. If there's a way this can occur, we should add it back with test coverage.

  5. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,127 @@
    +      // Wrap scripts_bottom placeholder with a marker.
    +      $content = str_replace($scripts_bottom_placeholder, '<drupal-big-pipe-scripts-bottom-wrapper>' . $scripts_bottom_placeholder . '<drupal-big-pipe-scripts-bottom-wrapper>', $content);
    

    Is it a marker or a wrapper? This is confusing.

  6. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,127 @@
    +   * Processes placeholders for HTML responses.
    

    Wrong comment.

  7. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,127 @@
    +  public static function getSubscribedEvents() {
    +    // Run after placeholder strategies.
    +    $events[KernelEvents::RESPONSE][] = ['onRespondEarly', 3];
    +    // Run as pretty much last subscriber.
    +    $events[KernelEvents::RESPONSE][] = ['onRespond', -10000];
    +    return $events;
    +  }
    

    These need better documentation.

  8. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,279 @@
    +    // Split it up in various chunks.
    +    $split = '<!-- X-RENDER-CACHE-BIG-PIPE-SPLIT -->';
    +    if (strpos($content, $split) === FALSE) {
    +      $split = '</body>';
    +    }
    +    $page_parts = explode($split, $content);
    +
    +    if (count($page_parts) !== 2) {
    +      throw new \LogicException("You need to have only one body or one <!-- X-RENDER-CACHE-BIG-PIPE-SPLIT --> tag in your html.html.twig template file.");
    +    }
    

    This seems like a nice-to-have.

    <!-- X-RENDER-CACHE-BIG-PIPE-SPLIT --> seems to exist primarily to allow templates to indicate useful boundaries to split at. But… what's the point of doing that here? At this time, we already have a fully response. The only things that aren't rendered yet are the placeholders. And these "split here please" markers won't do anything to speed things up?

    Removed this for now. Now only </body> is supported.

  9. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,279 @@
    +    // Support streaming on NGINX + php-fpm (nginx >= 1.5.6).
    +    header('X-Accel-Buffering: no');
    

    This is a header. Headers should be sent in sendHeaders().

    Moved this to HtmlResponseBigPipeSubscriber::onRespond(), where the BigPipeResponse object is constructed.

Wim Leers’s picture

FileSize
33.71 KB
3.39 KB
  1. +++ b/core/modules/big_pipe/big_pipe.module
    @@ -0,0 +1,80 @@
    +  // Store the settings for later usage.
    

    Very vague, confusing comment.

  2. +++ b/core/modules/big_pipe/big_pipe.module
    @@ -0,0 +1,80 @@
    +  if (isset($settings['bigPipeResponseMarker'])) {
    
    +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,134 @@
    +    $attachments['drupalSettings']['bigPipeResponseMarker'] = 1;
    
    +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,272 @@
    +      $elements['#attached']['drupalSettings']['bigPipeResponseMarker'] = 1;
    

    This can use a better, more explicit marker to indicate *what* the marker is for exactly.

  3. +++ b/core/modules/big_pipe/big_pipe.module
    @@ -0,0 +1,80 @@
    +  if (isset($settings['bigPipeResponseMarker'])) {
    

    This marker is no longer necessary after this point, so we should unset it. There's no point in sending this marker as a setting to the end user. This is a purely internal thing, and in fact just an ugly work-around to get the data we need.

  4. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,134 @@
    +    // Set a marker for our alter hook.
    
    +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,272 @@
    +      // Ensure that we update the ajaxPageState again.
    

    These comments are rather vague also.


See the interdiff. As the comment in the first hunk of the interdiff now makes abundantly clear, together with this issue comment and issue comment #131, the way that this BigPipe module module must deal with AJAX page state is not really acceptable. Not only is it extremely difficult to follow, it is also brittle.

yched’s picture

a BigPipe response is essentially an aggregated response: it consists of a HtmlResponse plus multiple AjaxResponses combined in a BigPipeResponse

Nice way to phrase it :-) That makes a lot of sense and gives a nice technical view of how BigPipe works regarding our existing ajax tools : an html response + N enclosed/streamed pre-resolved ajax responses, that will go through the same server-side (attachments handling) and client-side (behaviors) processing than if they had been initially requested by the client - if I get things right ?

I think that would deserve to be placed somewhere high in big_pipe.module comments and / or in the online docs :-)

Wim Leers’s picture

#137: indeed! While unraveling the whole AJAX page state mess to end up with a cleaner solution, I looked at all the processing and the intent, and ended up with this description, that I was planning to post later, but might as well post now:

BigPipe sends the page skeleton with the non-personalized parts of the page first. Let's call it The Skeleton. The personalized parts of the page are represented by BigPipe Placeholders that are replaced later.

The Skeleton of course can also have attachments. Including asset libraries. And those we track in drupalSettings.ajaxPageState.libraries — so that when we load new content through AJAX, we don't load the same asset libraries again. A HTML page can have multiple AJAX responses, each of which should take into account the combined AJAX page state of the HTML document and all preceding AJAX responses.

BigPipe does not use of multiple AJAX requests/responses. It uses a single HTML response. But it is a more long-lived one: The Skeleton is sent first, the closing </body> tag is not yet sent, and the connection is kept open. Whenever another BigPipe Placeholder is rendered, Drupal sends that by sending (and thus appending to the already sent HTML) something like <script type="application/json">[{"command":"settings","settings":{…}}, {"command":…}.

So, for every BigPipe placeholder, we send such a <script type="application/json"> tag. And the contents of that tag is exactly like an AJAX response. The BigPipe module has some JS that listens for these and applies them. Let's called it an Embedded AJAX Response (since it is embedded in the HTML response). Now for the interesting bit: each of those Embedded AJAX Responses must also take into account the combined AJAX page state of the HTML document and all preceding Embedded AJAX responses.

(IMO this description is ready to be used as-is in the docs.)

Still working on making all that better, as I said below the horizontal rule in #136.

yched’s picture

Cool ! The snippet quoted in #137 would IMO still be a nice TL;DR introduction to the detailed explanation in #138 ;-)
"html response + N embedded ajax 'replace' commands" feels like a fine mental takeaway.

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Needs documentation

+1!

Captured in the remaining tasks and issue tag.

Wim Leers’s picture

FileSize
41.54 KB
17.1 KB

So, AJAX page state! I think I've been able to arrive at an elegant solution.

First read the full-length explanation in #138. Then it becomes clear that dealing with AJAX page state for BigPipe means tracking the libraries that are already loaded by either The Skeleton or one of the preceding Embedded AJAX Responses.

The problem so far was that is was effectively impossible to get the final AJAX page state for a response. All previous patches used the hook_js_settings_alter() hack that mostly works, but doesn't work completely (see the paragraph below the horizontal rule in #136). So not only was it ugly and very hard to understand, it also couldn't work in all cases.

This patch fixes that, by:

  1. Requiring AssetResolver::getJsAssets() (which gathers the final JavaScript settings) by updating the AttachedAssetsInterface object it receives. Since that is the code calling hook_js_settings_build() and hook_js_settings_alter(), it also is the natural place for this.
  2. Requiring AttachmentsResponseProcessorInterface implementations to update the Response they are given to update that response's attachments to match the final values for those attachments.
  3. 1 + 2 = we no longer need hacks to track AJAX page state in BigPipe's logic.
Wim Leers’s picture

Issue summary: View changes

Update "API changes" section in IS for #141.

Wim Leers’s picture

Title: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » [PP-1] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
Component: cache system » request processing system
Wim Leers’s picture

FileSize
41.96 KB
1.71 KB

In this issue:

the 'messages' block does not play nicely with the session handler in the stage where BigPipe runs

And from IRC:

[2015-09-01 11:23:48] <WimLeers> The messages block works fine with BigPipe? I don't see why it wouldn't.
[2015-09-01 12:19:52] <Fabianx-screen> WimLeers Messges block won't work with BigPipe as the session is shutdown already - somehow - likely bug in session handler.

I just found the root cause of that bug.

The Session middleware does this after it has passed on the request to the HTTP kernel:

    if ($type === self::MASTER_REQUEST && $request->hasSession()) {
      $request->getSession()->save();
    }

Note that this happens before $response->send() happens, and thus before BigPipe streams the response. Which means that any changes to the session (such as messages having been shown and therefore removed from the session's data) that happen while BigPipe is rendering placeholders are lost.

Now, \Symfony\Component\HttpFoundation\Session\SessionInterface::save() has the following documentation:

    /**
     * Force the session to be saved and closed.
     *
     * This method is generally not required for real sessions as
     * the session will be automatically saved at the end of
     * code execution.
     */
    public function save();

Note how it says it's not required to be called.

Then if we do some archaeology to figure out why this is being called in the Session middleware, then we find #2229145: Register symfony session components in the DIC and inject the session service into the request object introduced this and at #30.5 it was asked to document why we're only saving the session for the master request, to which the answer was:

Well, I frankly do not know. This is what Symfony does in its SessionListener. Session is as global as something can be in PHP, so there is not much of a point dragging that through the request stack?

Therefore we can simply remove this.

Wim Leers’s picture

Title: [PP-1] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » [PP-2] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
almaudoh’s picture

#144/#145: You might want to ping @znerol on those. He did quite some work on session management.

Fabianx’s picture

#141 is exactly what I originally envisioned that responses and new page state are coupled! I just did not know how.

Great work!

nod_’s picture

FileSize
41.95 KB
6.28 KB

All right, spent the afternoon on the JS (I know, I know…) because I wanted to use mutation observer to process the new tags added to the page while they're loading. Turns out after I removed deflate mod from apache, and remembering to remove my ad blocking proxy I finally had chunked response from my server. Only to find out that IE doesn't want to play nice and that mutation observer are not possible to use for this use case on IE/Edge while it works fine in chrome/ff.

So I went back to the spirit of the original code and improved things while removing the excess of processing happening. Simplified the HTML as well. I don't see the need for a wrapping div, if it's needed elsewhere feel free to add it back but it doesn't matter for JS. The code doesn't remove the bigpipe script tags anymore, it's just dead text, I don't see why we have to remove it. Addressed a race condition that came up when parsing too quickly. I didn't keep the setInterval since we have no idea how much time the processing of an ajax request takes and it's very easy for them to take more than 100ms. I only set a timeout when the current request has finished to be processed. Having multiple ajax stuff going on at the same time does not make me comfortable.

I've turned it inside out and it should be all that is required for the JS to work.

yched’s picture

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -3,88 +3,90 @@
+        var response = JSON.parse(content);
+        // Use a dummy url.
+        var ajaxObject = Drupal.ajax({url: 'big-pipe/placeholder.json'});
+        ajaxObject.success(response);

It's a bit sad that we have to create a dummy ajaxObject with a dummy URL to be able to execute the "run commands" part of Drupal.Ajax.prototype.success()

Wouldn't it be possible to split that part out of Drupal.Ajax.prototype.success and make an "execute a set of Drupal.AjaxCommands" function publicly available ? That wouldn't be a JS API break, would it ?

[edit : yeah, but no, each command expects an ajax object, so we need to create one anyway. Sorry for the noise]

[edit : Also, #141 looks super elegant indeed. Awesome :-). I though I posted this earlier but didn't notice it got blocked by a crosspost]

yched’s picture

Also, that's probably a naive remark, but :
- instead of needing a timer to watch new chunks of JSON commands appearing in the DOM and pass them through bigPipeProcessPlaceholder(),
- why don't we directly make the chunks be <script>bigPipeProcessPlaceholder(jsonCommandsForThisChunk)</script> ?

[edit : Aw. Probably because we don't allow inline scripts :-p. Yeah, never mind then. Sad that mutation observers don't work instead of the timer]

Wim Leers’s picture

#146: Done: #2229145-138: Register symfony session components in the DIC and inject the session service into the request object.

#147: yay! That is great confirmation! Very glad to hear that :)

#148: awesome, thank you!

#149: oh yay, more happiness for #141 :)

#150: that's how Facebook used to do it, but it indeed prevents certain extra protection from being enabled. (I don't have the link handy on my iPad here, but I think you all know which issue I mean?)


I'll be on vacation until next Monday. This is a wonderful way to go into vacation: lots of happy comments, lots of progress :)

Wim Leers’s picture

Title: [PP-2] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » [PP-1] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
FileSize
41.97 KB
4.67 KB

The responses to #2597520: Don't save the session before $response->send() indicated that the session should simply be reopened for the duration of rendering placeholders in BigPipe. So, doing that instead.

One less blocker, yay!

Wim Leers’s picture

A common case is for a BigPipe placeholder to be rendered into the empty string. For example, for messages. We should optimize this common case.

It currently looks like this:

<script type="application/json" data-big-pipe-event="start"></script>
    <script type="application/json" data-big-pipe-placeholder="drupal-render-placeholder-callbackdrupalcorerenderelementstatusmessagesrendermessages-arguments0-tokena8c34b5edrupal-render-placeholder-" data-drupal-ajax-processor="big_pipe">
    [{"command":"settings","settings":{"ajaxPageState":{"theme":"seven","libraries":"big_pipe\/big_pipe,ckeditor\/drupal.ckeditor,ckeditor\/drupal.ckeditor.plugins.drupalimagecaption,classy\/base,classy\/image-widget,classy\/messages,comment\/drupal.comment,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/drupal.autocomplete,core\/drupal.collapse,core\/drupal.date,core\/drupal.dropbutton,core\/drupal.states,core\/html5shiv,core\/jquery.form,core\/normalize,embed\/embed,file\/drupal.file,filter\/drupal.filter,menu_ui\/drupal.menu_ui,node\/drupal.node,path\/drupal.path,seven\/global-styling,seven\/node-form,shortcut\/drupal.shortcut,text\/drupal.text,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons","theme_token":"_uIazomhVNnuNYqdIspp-9i3YRW-Ay9emJxRC4licb8"},"pluralDelimiter":"\u0003","user":{"uid":"1","permissionsHash":"e938424be3ec420bb0ff652336217dcb046ebbc5d84ba6bee5b216c04c7ca86f"}},"merge":true},{"command":"add_css","data":"\u003Clink rel=\u0022stylesheet\u0022 href=\u0022http:\/\/tres\/core\/themes\/classy\/css\/components\/messages.css?nwtqsm\u0022 media=\u0022all\u0022 \/\u003E\n"},{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022drupal-render-placeholder-callbackdrupalcorerenderelementstatusmessagesrendermessages-arguments0-tokena8c34b5edrupal-render-placeholder-\u0022]","data":"\n","settings":null}]
    </script>
<script type="application/json" data-big-pipe-event="stop"></script>

Note:

  • "data:"\n"
  • the entire ajaxPageState is updated… to be exactly the same as before, plus the classy/messages library, which core/themes/classy/templates/misc/status-messages.html.twig attaches.
  • … but that asset library is useless if no messages are shown!

But… turns out simply changing the template to only attach the library when necessary actually fixes it:

<script type="application/json" data-big-pipe-event="start"></script>
    <script type="application/json" data-big-pipe-placeholder="drupal-render-placeholder-callbackdrupalcorerenderelementstatusmessagesrendermessages-arguments0-tokena8c34b5edrupal-render-placeholder-" data-drupal-ajax-processor="big_pipe">
    [{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022drupal-render-placeholder-callbackdrupalcorerenderelementstatusmessagesrendermessages-arguments0-tokena8c34b5edrupal-render-placeholder-\u0022]","data":"  ","settings":null}]
    </script>
<script type="application/json" data-big-pipe-event="stop"></script>

… which is actually as expected: the settings are only updated if they're actually different.

Hurray! This means there's nothing to do here :)

Opening an issue to fix that template.

Wim Leers’s picture

Wim Leers’s picture

FileSize
40.67 KB
5.16 KB
+++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
@@ -80,6 +80,8 @@ public function createPlaceholder(array $element) {
+      // The options for creating the placeholder. (optional)
+      '#create_placeholder_options' => TRUE,

+++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
@@ -0,0 +1,107 @@
+    foreach ($placeholders as $placeholder => $placeholder_elements) {
+      // Blacklist some #lazy_builder callbacks.
+      // @todo Use #create_placeholder_options instead.
+      if (isset($placeholder_elements['#lazy_builder'][0])) {
+        // Route CSRF tokens, form CSRF token and form actions are (part of)
+        // HTML attributes, not HTML.
+        if ($placeholder_elements['#lazy_builder'][0] == 'route_processor_csrf:renderPlaceholderCsrfToken') {
+          continue;
+        }
+        elseif ($placeholder_elements['#lazy_builder'][0] == 'form_builder:renderPlaceholderFormAction') {
+          continue;
+        }
+        elseif ($placeholder_elements['#lazy_builder'][0] == 'form_builder:renderFormTokenPlaceholder') {
+          continue;
+        }
+      }

IMO this is the biggest remaining problem.

We currently have three placeholders that do not represent regular HTML. In all of these cases, these are placeholders for HTML tag attribute values or parts thereof.

This means that we cannot generate BigPipe placeholders for them, because we would not be able to use JavaScript/the DOM to find them efficiently. We'd literally have to parse the entire DOM in JS to find the text node where this lives. Perhaps we could do it using XPath, but that'd be tremendously slow, because we'd have to check every single DOM text node. That's not an option.

However, as #2593481: PlaceholderGenerator::createPlaceholder() generates invalid markup; causes placeholders to not be replaced if processed by DOMDocument already has demonstrated, placeholders must already result in valid HTML. This means that placeholders for HTML fragments already MUST be valid HTML (<drupal-render-placeholder …>) and placeholders for other things, such as HTML attribute values, MUST NOT be HTML (the 3 highlighted ones).

Therefore, a super simple, super elegant solution is actually staring us in the face :)

The solution: check if $placeholder is valid HTML. If it is, it can be converted into a BigPipe placeholder. If it is not, it cannot be, and it will have to be replaced on the server side.

Wim Leers’s picture

Now there are IMO 3 things left:

  1. HalfPipe: how to deal with that more elegantly. It's currently too deeply intertwined, too hard to understand IMO.
  2. Since #131, this patch dispatches the RESPONSE event and then relies on the AjaxResponseSubscriber to do the necessary attachment processing. That's all using \Drupal::service() and should really use proper injection.
  3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -141,9 +142,10 @@ public function processAttachments(AttachmentsInterface $response) {
    +    // @todo This needs to be configurable somehow.
         $unsupported_types = array_diff(
           array_keys($attached),
    -      ['html_head', 'feed', 'html_head_link', 'http_header', 'library', 'html_response_attachment_placeholders', 'placeholders', 'drupalSettings']
    +      ['html_head', 'feed', 'html_head_link', 'http_header', 'library', 'html_response_attachment_placeholders', 'placeholders', 'drupalSettings', 'big_pipe_placeholders']
         );
    

    This is the last remaining problem. This may require an API addition. Not sure yet how to support this elegantly.

And then: polish, polish, polish.

yched’s picture

And the approach for big_pipe_has_js outlined in #100 ?

Wim Leers’s picture

We (Fabianx, effulgentsia, moshe.weitzman and I) had a call yesterday. Outcomes:

  1. Move this to a contrib module as soon as the blockers uncovered by this issue/patch are solved. Project page is already created and points to this issue until then: https://www.drupal.org/project/big_pipe
  2. We're going to continue to call it "BigPipe" because that's A) what Facebook called it, and this implements the same architecture, B) that's what we've been calling this for >1 year now, C) any other name will be either equally nonsensical or far too technical, so better to stick with a name that many people already know.

(And I'm sure I'm forgetting some things, but those are definitely the most important bits.)

Wim Leers’s picture

FileSize
41.57 KB
8.44 KB

This addresses #156.2: That's all using \Drupal::service() and should really use proper injection.

Also adds docs that will make it much easier to understand.

Wim Leers’s picture

FileSize
46.17 KB
7.19 KB

This addresses #156.3: This is the last remaining problem. This may require an API addition. Not sure yet how to support this elegantly.

Discussed this with Fabianx. The solution for now is to decorate the default HTML response attachment subscriber with our own, which pulls out the BigPipe placeholders, so that the default one doesn't complain about unknown attachment types.

In the future, Drupal 8 could add an API to make it possible to define new attachment types. For now, this is totally acceptable. It just requires a small bit of decorating and a small bit of duplication to make this possible.

Wim Leers’s picture

FileSize
46.96 KB
1.61 KB

#107 explained how we can ensure that reverse proxies pass BigPipe responses correctly: Surrogate-Control: content="BigPipe/1.0". This implements that.

It also ensures the response has Cache-Control: private, to ensure regular proxies ignore it.

Wim Leers’s picture

FileSize
47.44 KB
1.42 KB

Small improvement: properly document \Drupal\big_pipe\Render\BigPipe::renderPlaceholder().

Wim Leers’s picture

@yched, @Fabianx, @nod_: Could you review #2597359: Require responses with attachments to contain the final attachment values? I know you've already posted general +1s here, but now we need to actually get that in core. It's the reason this is currently "[PP-1]", so would be great to get that out of the way :)

tstoeckler’s picture

+++ b/core/modules/big_pipe/src/Render/BigPipeResponseAttachmentsProcessor.php
@@ -0,0 +1,107 @@
+class BigPipeResponseAttachmentsProcessor extends HtmlResponseAttachmentsProcessor {

Hmm... would indeed be nice to have something like a chained attachments processor where each one picks up the one it wants. Could use the standard "service_collector" pattern. Because overriding core services is kind of hardcore for contrib modules (OTOH very legit for custom modules) because as soon as you have two modules overriding the same service randomness/hilarity ensues.

Wim Leers’s picture

because as soon as you have two modules overriding the same service randomness/hilarity ensues.

Not in this case. Note how it decorates the existing service. This service itself could be decorated again, or it could decorate an already-decorated service, and it'd all still work.

But, yes, it'd be nice to improve the architecture there, but given this is the only example for now, and we're in RC, this is a bad time to add more abstractions. We need >1, preferably >2 examples to make good architecture decisions.

So: IMO fine at this time, but +1 for improving in the future.

cosmicdreams’s picture

Look like in #108 someone asked about Web Components. I happen to have experience with web components so let me try to answer. I should also say that I also read Wim's excellent description of the current solution in 138 and Fabian's description of the challenges in 107. I think @skyredwang was onto something:

First BigPipe:

It sounds like what we're trying to accomplish with Big Pipe is to deliver the critical assets / structure of a page as quickly as possible. These assets could be context free, meaning no personalized components or things that require the server to think hard about creating the components before delivering them to the user. In other words things that are likely going to be cached and can live in the cache for a long time. Therefore delivered fast.

In order to know where the components are eventually going to render, they are replaced with placeholders. After the page is delivered, additional requests deliver the data for the components as a <script type="application/json"></script> and then, I guess, trigger the components to populate themselves with data.

As the issue description states, there are many ways to replace the placeholders, that solution could be Web Components. Let's explore what that would mean:

1. Components are internal coherent.

All the assets, behaviors, maybe even business logic is stored with the component. #108 mentioned HTML Imports as a delivery / file organization system for these components but ES6 modules may rise to fit this task. What is practically means is that the component is responsible for delivering the right assets at the right time for each component.

2. We can load inert components before they're ready for data.

We can load the markup into the DOM, and all of the assets needed, without activating the components and populating them with their data.

3. The approach can mostly be the same.

We can still deliver the data as json, If we could have a Object Observer / Listener pattern or Object Mutator pattern that would be cool to. But we could simple deliver the JSON to the bottom of the page then have the components go get their data.

4. Web components can still be done without all the help

It would be cool if this could be reused for a solution that included Web Components. Other ways of implementing web components is to have the components themselves execute a REST API data access request (individually) and that feels expensive in light of this slim-lined / intelligent solution.

Summary:

So yes web components could be used. And support in browsers is growing with even MS getting on board with support and more support to come. Not all browsers will likely support the full standard and the web is always moving. It's important that we are able to adapt quickly if this system that's being built becomes obsolete due to new solutions to this problem.

Futher reading:
https://blogs.windows.com/msedgedev/2015/07/15/microsoft-edge-and-web-co...
http://webcomponents.org/articles/introduction-to-html-imports/
https://hacks.mozilla.org/2014/12/mozilla-and-web-components/

Wim Leers’s picture

#166: I replied this to #108 in #110:

#108: that's exactly the direction I was thinking in also :), but none of that is in a mature state yet, and definitely not within Drupal. So I think it will make sense to evolve in that direction, but we shouldn't want to do that here; it'd expand the scope far too much.

I still stand by that analysis. I think using web components will make sense once web components themselves mature. But right now, using Web Components inherently means loading much more JS (not to mention that Web Components themselves use JS), and the benefit is extremely minimal. In essence, we'll still need everything this patch has today. We'll just need more code.

Therefore this is a nice-to-have. So, filed an issue for that, linking to #108 and #166: #2602726: As an alternate to BigPipe, add a PlaceholderStrategyInterface implementation that renders placeholders as web components. Feel free to expand that issue.

This allows us to continue to focus on just finishing BigPipe here.

Wim Leers’s picture

FileSize
47.36 KB
1.23 KB

Simplification: placeholders are already unique, no need for tokens. Even more so because the token currently always was the empty string :)

Fabianx’s picture

+++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
@@ -96,6 +96,21 @@ public function onRespond(FilterResponseEvent $event) {
     $big_pipe_response->setContent($response->getContent());
     $big_pipe_response->addCacheableDependency($response->getCacheableMetadata());
...
+    $response->setPrivate();
...
+    $response->headers->set('Surrogate-Control', 'no-store, content="BigPipe/1.0"');

Should be $big_pipe_response here.

I am not sure how good this is clonable ...

It probably would just work as we clone the headers later, but ...

cosmicdreams’s picture

Oh cool, I read through this issue and didn't find that one. Skimming--

I don't want distract from the progress made here ( great work BTW). Just wanted to highlight that the problem of asset handling is greatly simplified by web components as the components can be held responsible for managing their own assets, and bonus points for doing this in a non-Drupal, non-PHP kind of way (broader audience?). Fabian called that out as a technical issue in #107, which is what probably prompted someone to mention Web Components.

I'll move the convo about WC to the issue you linked.

Wim Leers’s picture

FileSize
50.73 KB
7.34 KB

An important thing I forgot in #158: on that same call, we decided to call the JS-less version of BigPipe not "HalfPipe" (as the current code does) nor "SmallPipe" nor anything like that. Just "no-JS BigPipe". And in fact, not even expose it. Just present it all as "BigPipe". Exposing those details and inventing new names just makes it all the more confusing.

What follows is in light of that.


An important step in addressing #156.1 (HalfPipe: how to deal with that more elegantly. It's currently too deeply intertwined, too hard to understand IMO.).

(I've already got more work done locally, but I'm trying to make sure there are simple, clear steps to get there.)

A big problem in the current code is that it's either no-JS BigPipe ("HalfPipe" in current code) or JS BigPipe. i.e. we have just one set of placeholders, and they're either replaced using JS or using flushing. And either is selected depending on whether the current session has JS or not.

This prevents two things:

  1. Clarity.
  2. The ability to use both simultaneously.

So, in this reroll:

  • Added a lot of documentation to BigPipeStrategy to explain the difference between the two, why either one is preferred in different situations, and why in fact it's useful to be able to use both simultaneously. (Second hunk.)
  • To prepare for this, add a createBigPipeJsPlaceholder() helper. A no-JS one will follow.
  • In doing so, I noticed that the selector is in fact a very, very nasty string that's hard for humans to recognize or even look at. (It's as if Fabian put a fork in my eye, really :P) So, instead of using Html::getId($placeholder) as the selector, this now generates a selector that is far easier on the eyes and contains the same metadata.
Wim Leers’s picture

FileSize
50.75 KB
1.47 KB

#169: good catch, fixed.

Wim Leers’s picture

FileSize
51.32 KB
2.7 KB

@Fabianx pointed out in IRC that since #171 it'd be impossible to have placeholders with non-lazy-builder-based render arrays. He was +1 for the DX improvement #171 brought, but we should still support the old behavior as a fallback.

Wim Leers’s picture

FileSize
50.93 KB
4.52 KB

There's no point in injecting the BigPipe placeholders separately into the BigPipe response, if it can already access the response's attachments, which contains those very placeholders.

(That's a remnant from one of the very early iterations of this patch.)

And since the BigPipe service does need all the attachments anyway, and already receives them from the BigPipe response, there's also no point in passing them there separately.

Hence this is a nice bit of simplification.

Wim Leers’s picture

FileSize
52.18 KB
6.2 KB

This finishes what #171 started in terms of structure:

An important step in addressing #156.1 (HalfPipe: how to deal with that more elegantly. It's currently too deeply intertwined, too hard to understand IMO.).

Next steps (tomorrow):

  • Make the no-JS BigPipe rendering ("HalfPipe") much more understandable, and similar to how the regular (JS) BigPipe rendering works.
  • Make it possible to use both simultaneously.
hass’s picture

I is possible for a module to opt-out from bigpipe? I have not tested the patch, but if Google Analytics/Piwik tracking is added it should stay in header untouched and do not get removed and loaded async with bigpipe / via JS - just as one example. There may not many situations like these, but they exists and require a solution I think.

Fabianx’s picture

#176: Unless the modules code is placeholdered, it will never get in touch with BigPipe. So as long as either hook_js_settings_alter(), hook_page_attachments() or any other hook affecting the whole page is used, this is fine.

Only if GA was suddenly attached within a block and that block would be placeholdered due to it being uncacheable then could it in theory happen that things that are in the header would suddenly move directly in front of the element.

So should not affect GA at all.

Wim Leers’s picture

#176: Like #177 already indicated: no, GA will be completely unaffected. Header JS that is attached not to one of the dynamically delivered parts of the page will still just load in the header. :)

Wim Leers’s picture

FileSize
52.35 KB
3.8 KB

Clean-up:

  • Add docs to HtmlResponseBigPipeSubscriber.
  • Slightly improve docs for BigPipeInterface; final version once #156.1 is completed.
  • Significantly improve docs for BigPipeResponse.
  • Remove @todo Rename #attached[big_pipe_placeholders] to #attached[big_pipe_js_placeholders], which was added in #171. Discussed with @Fabianx, we agree it's better to just call the "JS BigPipe placeholders" simply "BigPipe" placeholders, because that's what they really are. BigPipe originally is really JS-based. It's the no-JS variant that we're adding that's not in the original. So only that one should be called out separately, specifically.
Wim Leers’s picture

FileSize
58.71 KB
23.54 KB

This is the biggest, most important refactoring/clean-up so far. This is the final thing that was in the way of cleaning up no-JS BigPipe ("HalfPipe" previously).

This should make BigPipe much more understandable:

  • BigPipeInterface now has comprehensive documentation!
  • BigPipe::sendContent() now calls out to helper methods, that make the code flow (and response flow!) much easier to understand.
  • Note that the interdiff is very hard to follow; it's much easier to just look at BigPipe before and after.

This is what BigPipe::sendContent() now looks like:

  public function sendContent($content, array $attachments) {
    // First, gather the BigPipe placeholders that must be replaced.
    $placeholders = isset($attachments['big_pipe_placeholders']) ? $attachments['big_pipe_placeholders'] : [];
    $nojs_placeholders = isset($attachments['big_pipe_nojs_placeholders']) ? $attachments['big_pipe_nojs_placeholders'] : [];

    // BigPipe sends responses using "Transfer-Encoding: chunked". To avoid
    // sending already-sent assets, it is necessary to track cumulative assets
    // from all previously rendered/sent chunks.
    // @see http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.41
    $cumulative_assets = AttachedAssets::createFromRenderArray(['#attached' => $attachments]);
    $cumulative_assets->setAlreadyLoadedLibraries(explode(',', $attachments['drupalSettings']['ajaxPageState']['libraries']));

    // The content in the placeholders may depend on the session, and by the
    // time the response is sent (see index.php), the session is already closed.
    // Reopen it for the duration that we are rendering placeholders.
    $this->session->start();

    list($pre_body, $post_body) = explode('</body>', $content, 2);
    $this->sendPreBody($pre_body, $nojs_placeholders, $cumulative_assets);
    $this->sendPlaceholders($placeholders, $this->getPlaceholderOrder($pre_body), $cumulative_assets);
    $this->sendPostBody($post_body);

    // Close the session again.
    $this->session->save();

    return $this;
  }
Wim Leers’s picture

FileSize
65.49 KB
14.75 KB

This then finally fully addresses #156 (HalfPipe: how to deal with that more elegantly. It's currently too deeply intertwined, too hard to understand IMO.)! :)

Changes:

  • No-JS BigPipe placeholders (again, formerly "HalfPipe") are now replaced using embedded HTML responses, which is then perfectly analogous to how BigPipe placeholders are replaced using embedded AJAX responses. So this constructs HtmlResponse objects and invokes the RESPONSE event on it as if it were a subrequest, just like I did for "regular" BigPipe in #131.
  • [core change] HtmlResponseSubscriber should react to all requests: both master and sub requests. Otherwise, attachments would not be processed for those embedded HTML responses. I will open a new core issue for this.
  • Related yet differently, HtmlResponseBigPipeSubscriber has two RESPONSE event callbacks: onRespondEarly() and onRespond(). But only the latter returns early for subrequests. Of course turning responses for subrequests into BigPipe responses makes zero sense. So both should return early for the same reasons. Rather than duplicating the same logic, I applied the same pattern as DynamicPageCacheSubscriber uses: calculate it in the first event callback, store the result in a request attribute, and then reuse it.
  • In HEAD, any no-JS placeholder does NOT get its critical CSS/JS (CSS + header JS) printed before its markup. This makes it possible for the no-JS BigPipe placeholder strategy to break the rendered output. So, fixed that. Of course, we still gather the libraries and settings for all no-JS BigPipe placeholders and then re-render scripts_bottom at the very end if necessary.
  • Finished BigPipeInterface's documentation for no-JS BigPipe placeholders.

Not a single mention of "HalfPipe" is left.

Wim Leers’s picture

FileSize
66.58 KB
4.1 KB

And finally, as I promised yesterday in #175:

Make it possible to use both simultaneously.

Here we go :)

Changes:

  1. Make the no-JS BigPipe placeholder's selector different than the BigPipe placeholder's selector. Otherwise we cannot parse out just the no-JS BigPipe placeholders, and things break horribly.
  2. Placeholders that were NOT replaceable until now (those in HTML attribute values), now can be mapped to no-JS BigPipe placeholders! It's not possible to replace them using JavaScript, but it's totally possible to do it using no-JS BigPipe placeholders. This means that even expensive form pages for example can benefit from BigPipe in a more meaningful way: we can send the head of the page immediately, without first having to replace those using the SingleFlushStrategy!
  3. Updated BigPipeInterface's docs once more, this time with a complete schematic of how sending a response with both no-JS BigPipe placeholders and BigPipe placeholders works.

See both combined in action over at /node/add/article, for example :)

Wim Leers’s picture

Issue tags: -Needs documentation
FileSize
66.59 KB
17.49 KB

And here's then another clean-up round: some renaming of variables, some moving of methods, removing of unused use statements, etc.

AFAIK that leaves just one important thing remaining: supporting anonymous users. But we already have an issue for that: #2603046: Support anonymous users.

And, of course, tests. That's to be added once this moves to contrib.

Wim Leers’s picture

FileSize
1.86 KB
66.66 KB

There was one bug that I found some time ago, but can now solve elegantly. When replacing BigPipe placeholders, it's possible that no libraries or settings are added. In that case, there's nothing to update $cumulative_assets with. The code was not yet handling that edge case. Fixed.

Also now pointing to the aforementioned #2603046: Support anonymous users in the code.

(Also: rebased. Clean rebase, no conflicts.)

And with that, BigPipe is IMO now DONE! Time to remove the POC suffix in the filename :)

Dom.’s picture

@Wim Leers: Reading this issue comments days after days... so this is just a (huge) congratulation for all your work on this so far :)
Sorry for I have nothing more constructive to add on this issue !!

Fabianx’s picture

Assigned: Wim Leers » Fabianx

I am gonna give this a review tomorrow.

Wim Leers’s picture

Title: [PP-1] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » [PP-3] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
almaudoh’s picture

Excellently done @Wim Leers and @Fabianx!! Looking forward to having this in D8 contrib.

Wim Leers’s picture

Title: [PP-3] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » [PP-2] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
Wim Leers’s picture

Title: [PP-2] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » [PP-1] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts
Wim Leers’s picture

Straight reroll of #184, basically a rebase on top of HEAD, now that with #2603786: Make Renderer(Interface)::renderPlaceholder() public (which used to be #132) and #2597359: Require responses with attachments to contain the final attachment values (used to be #31) are committed.

This has cut patch size from 67 to 55 KB. :)

And it leaves us with this diffstat:

$ git d origin/8.0.x HEAD
 core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php   |   4 -
 core/modules/big_pipe/big_pipe.info.yml                           |   6 +
 core/modules/big_pipe/big_pipe.libraries.yml                      |  11 +
 core/modules/big_pipe/big_pipe.module                             |  56 ++++
 core/modules/big_pipe/big_pipe.services.yml                       |  20 ++
 core/modules/big_pipe/js/big_pipe.js                              |  92 +++++++
 .../src/EventSubscriber/HtmlResponseBigPipeSubscriber.php         | 162 ++++++++++++
 core/modules/big_pipe/src/Render/BigPipe.php                      | 427 ++++++++++++++++++++++++++++++
 core/modules/big_pipe/src/Render/BigPipeInterface.php             | 131 +++++++++
 core/modules/big_pipe/src/Render/BigPipeResponse.php              |  50 ++++
 .../big_pipe/src/Render/BigPipeResponseAttachmentsProcessor.php   | 115 ++++++++
 core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php  | 192 ++++++++++++++
 12 files changed, 1262 insertions(+), 4 deletions(-)

… in other words, only 4 deletions left in this patch that modify core (for which we have #2603788: HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests); the rest is the big_pipe module will live in contrib until at least 8.1.x — see https://www.drupal.org/project/big_pipe.

Fabianx’s picture

  1. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,427 @@
    +    list($pre_scripts_bottom, $scripts_bottom, $post_scripts_bottom) = explode('<drupal-big-pipe-scripts-bottom-marker>', $pre_body, 3);
    

    nit - we should use a constant for the MAGIC string.

  2. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,427 @@
    +    if ($cumulative_assets_initial != $cumulative_assets) {
    

    Does that comparison work?

  3. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,427 @@
    +          'library' => $cumulative_assets->getAlreadyLoadedLibraries(),
    

    To only put the already loaded libraries in here feels wrong, but I don't know the asset system well enough to say that this is right or wrong off-hand.

  4. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,427 @@
    +    ob_end_flush();
    

    nit - We only need one ob_end_flush() once.

  5. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,427 @@
    +      // @todo What if drupalSettings already was printed in the HTML <head>? That case is not yet handled. In that case, no-JS BigPipe would cause broken (incomplete) drupalSettings… This would not matter if it were only used if JS is not enabled, but that's not the only use case. However, this
    

    Note: No JS BigPipe should just return in the case that settings are in the HEAD. We already know that property in the attachments processor and its early enough to abort then.

  6. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,427 @@
    +      $fake_request->request->set('ajax_page_state', ['libraries' => implode(',', $cumulative_assets->getAlreadyLoadedLibraries())] + $cumulative_assets->getSettings()['ajaxPageState']);
    +      $this->requestStack->push($fake_request);
    +      $event = new FilterResponseEvent($this->httpKernel, $fake_request, HttpKernelInterface::SUB_REQUEST, $ajax_response);
    +      $this->eventDispatcher->dispatch(KernelEvents::RESPONSE, $event);
    +      $ajax_response = $event->getResponse();
    +      $this->requestStack->pop();
    

    nit - Even though it varies on the details, there is some code duplication here.

This looks fantastic on first review. I don't think core needs any more changes than the one we have; the rest is pure module.

I think we can indeed continue to refine in contrib and I think we'll be moving over there this week.

Wim Leers’s picture

#192:

  • 2. Yes. I tested it with the debugger.
  • 3. What feels wrong about it? It includes all assets sent so far, including the ones sent by no-JS BigPipe.
  • 5. Exactly. Which is why I didn't fix this yet; we'll want test coverage for that case anyway. But indeed, we know we can detect it early enough.
Wim Leers’s picture

Title: [PP-1] BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts » BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts

#2603788: HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests landed also, which means BigPipe is now fully unblocked in 8.0.0 contrib!

(Also see #2603788-38: HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests specifically, where a small additional bugfix was made: this patch did not work for placeholders attached to the response for a subrequest! So the comment form for /comment/1 was not delivered via BigPipe. Now it will be :))

Wim Leers’s picture

FileSize
54.15 KB

And a straight rebase, with #2603788: HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests omitted automatically, thanks to git's 3-way rebase support!

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed

This patch was mapped to the BigPipe contrib module project, every comment here corresponds to a commit there, including proper history. Thanks to the work here, we just tagged beta1 of the BigPipe module for Drupal 8.0.x: https://www.drupal.org/node/2619070.

Go and give it a try!

Postponing to revisit this issue later, to consider including it in Drupal 8.1.0.

yched’s picture

Opend #2619092: Get rid of $_SESSION['big_pipe_has_js'] ? in big_pipe issue queue for #100 so that the idea doesn't get lost :-)

Wim Leers’s picture

Assigned: Fabianx » Wim Leers
Issue summary: View changes
Status: Postponed » Active

Hello again everybody!

We're now slightly more than two months further. Time for an update.

  1. The BigPipe module for Drupal 8 has been released, and all feedback in this issue has been incorporated.
  2. Most significant change since this patch: on December 3 2015, we added support for anonymous users to the BigPipe module: #2603046: Support anonymous users.
  3. For weeks, no bugs have been reported. ~150 sites have been testing BigPipe, with no bug reports coming in. 150 isn't as good as ten thousand sites testing it, but it's still a significant number.
  4. Yesterday, I released beta3 of the module, it only includes documentation improvements since beta2.
  5. The most significant bit of documentation is the need to disable buffering on web servers running PHP via FastCGI. Prime example of this: simplytest.me. They adjusted that setting, and now BigPipe works on simplytest.me too.
  6. I've also updated https://www.drupal.org/developing/api/8/render/pipeline back on November 19 (Drupal 8 release day) to explain how BigPipe works from a render pipeline POV.
  7. The code documentation is already great in my opinion.
  8. It'd be great to get this into Drupal 8.1 as an experimental module.
  9. Depending on the feedback during the 8.1 cycle, we could then make it non-experimental in 8.2, and perhaps enable it by default in 8.3.

Remaining tasks:

  1. Add SessionExistsCacheContext to Drupal 8 core in a separate issue; it's useful to more than just BigPipe, and should therefore not live in the BigPipe module
  2. Test coverage
  3. Refine README
  4. Make it easy to set up a demo, and provide a live demo: #2627016: SimplyTest.me demo + live demo
  5. Reviews

Issue summary updated.

Wim Leers’s picture

Status: Active » Needs review
FileSize
65.18 KB
349 bytes

Attached are:

  • the core patch to review
  • a script that when ran from Drupal core automatically recreates the patch based on the BigPipe contrib module's latest version — any code changes proposed here I will first commit there, so that Drupal 8.0.x sites can benefit from the improved code also
Wim Leers’s picture

Category: Task » Feature request
swentel’s picture

Something that we'll need to think about is: upgrade path. If someone upgrades to 8.1 and has bigpipe as a contrib, extension discovery will detect the contributed version, not the core version. We're having the same problem in #2296423: Implement layout plugin type in core. I've planted this idea in alex's brain, still thinking myself as well how'd handle this gracefully.

dawehner’s picture

IMHO we should tell people that in the readme. 8.1 is not meant to be fire and go, like it has maybe some new features, which you might want to use for example.
This sounds more like a communication problem for me.

An alternative could be to rely on version restrictions while parsing the extensions, but IMHO this would be way more effort on every scanning.

colan’s picture

Or we could just change the name from "big_pipe" to "bigpipe" for core. So when "bigpipe" gets enabled, copy the settings over, and then print a message that the contrib one "big_pipe" should be uninstalled, or just do it.

Wim Leers’s picture

The first actual bug was reported: #2654386: [PP-1] BigPipe uses AJAX system to insert new content, AJAX system adds a wrapping <div>, breaks theme. But it's not a bug in the BigPipe module itself, it's a bug in the AJAX framework, which BigPipe relies upon. In fact, it's a bug that was reported back in 2010, but has grown in importance in Drupal 8 because it's now triggered for (almost?) every piece of content inserted by the AJAX system: #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs.

See #736066-62: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs.

andypost’s picture

forms has another issue with ajax

Wim Leers’s picture

#201/#202/#203: RE: moving a module from contrib into core affecting sites using that contrib module: I think we could further mitigate this by making the contrib module state core: 8.0.x in its *.info.yml file. But I'm not sure whether that is actually re-validated upon updating Drupal core.

tstoeckler’s picture

Re Wim Leers: I don't think that works, see #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning.

What should work (although admittedly I haven't tried it) is declaring a dependency on system (<8.1.0).

Wim Leers’s picture

Issue summary: View changes

I've now made it easy to look at a demo of BigPipe: https://www.drupal.org/project/big_pipe_demo


#207: thanks, great information!

Wim Leers’s picture

Update

  1. #198 said:

    Add SessionExistsCacheContext to Drupal 8 core in a separate issue; it's useful to more than just BigPipe, and should therefore not live in the BigPipe module

    I've now written test coverage for that new cache context (see #2671958: SessionExistsCacheContext test coverage) and have created a core patch that is ready for final review: #2671988: Add SessionExistsCacheContext ('session.exists'). That will make this patch again a bit smaller.

  2. #198 also said:
    Make it easy to set up a demo, and provide a live demo: #2627016: SimplyTest.me demo + live demo

    This is also done!
    Try the live demo at http://bigpipe.demo.wimleers.com!


Next

I'm currently working on test coverage for the BigPipe module itself and will continue to do so in the next week. Expect a new patch with full test coverage here next week.

Wim Leers’s picture

Finally!

Here it is. A BigPipe patch that includes full test coverage. That no longer includes the session.exists cache context, because that already landed in #2671988: Add SessionExistsCacheContext ('session.exists'). That now updates MAINTAINERS.txt and now has a hook_help() implementation.

(Again a script included to reproduce the patch.)

Four notes:

  1. The patch basically doubles in size compared to #199, but that's entirely due to the added test coverage :)
  2. @Fabianx has reviewed & RTBC'd all test coverage I added
  3. @nod_ wrote all JS and has RTBC'd any JS changes I made (pure docs and naming changes he did not RTBC, but since the logic stayed the same, that is also not necessary)
  4. >300 sites have been running the BigPipe module for over a month now, and we have not received any bug reports
Wim Leers’s picture

This was tagged API addition in #132, but that was already handled in #2603786: Make Renderer(Interface)::renderPlaceholder() public, so that tag is no longer relevant.

A tag that's missing, is front-end performance.

And finally, BigPipe 8.x-1.0 RC1 for Drupal 8.0 was also tagged, and is equivalent with the patch above: https://www.drupal.org/node/2676588.

dawehner’s picture

This is by far the best documented module I've seen. Really great work!

  1. +++ b/core/modules/big_pipe/README.md
    @@ -0,0 +1,113 @@
    +
    +- When using Apache+`mod_fcgid`, [set `FcgidOutputBufferSize` to `0`](https://httpd.apache.org/mod_fcgid/mod/mod_fcgid.html#fcgidoutputbuffersize):
    +```
    +<IfModule mod_fcgid.c>
    +  FcgidOutputBufferSize 0
    +</IfModule>
    +```
    +- When using Apache+`mod_fastcgi`, [add the `-flush` option to the `FastCGIExternalServer` directive](http://www.fastcgi.com/mod_fastcgi/docs/mod_fastcgi.html#FastCgiServer):
    +```
    +<IfModule mod_fastcgi.c>
    +  FastCGIExternalServer /usr/sbin/php5-fpm -flush -socket /var/run/php5-fpm.sock
    +</IfModule>
    +```
    +- When using Nginx+FastCGI, [set `fastcgi_buffering` to `off`](http://nginx.org/en/docs/http/ngx_http_fastcgi_module.html#fastcgi_buffering).
    +
    +
    +## IIS
    +
    +When using IIS, you must [disable its buffering](https://support.microsoft.com/en-us/kb/2321250).
    +
    +## Varnish
    +
    +When using Varnish, the following VCL disables buffering only for BigPipe responses:
    +
    +```
    +vcl_backend_response {
    +  if (beresp.Surrogate-Control ~ "BigPipe/1.0") {
    +    set beresp.do_stream = true;
    +    set beresp.ttl = 0s;
    +  }
    +}
    +```
    +
    +and for Varnish <4:
    +
    +```
    +vcl_fetch {
    +  if (beresp.Surrogate-Control ~ "BigPipe/1.0") {
    +    set beresp.do_stream = true;
    +    set beresp.ttl = 0;
    +  }
    +}
    

    Is this something you can check in a hook_requirements implementation?

  2. +++ b/core/modules/big_pipe/src/Controller/BigPipeController.php
    @@ -0,0 +1,64 @@
    +    if (!$request->query->has('destination')) {
    +      throw new HttpException(500, 'The original location is missing.');
    

    Semantically 500 are server errors, but this sounds much more like a client error (4XY)

  3. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,432 @@
    +        '#markup' => Markup::create($js_bottom_placeholder),
    

    It just feels a bit odd to use this internal class instead of shipping with a custom Markup implementation.

  4. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,432 @@
    +      $fake_request = $this->requestStack->getMasterRequest()->duplicate();
    +      $this->requestStack->push($fake_request);
    +      $event = new FilterResponseEvent($this->httpKernel, $fake_request, HttpKernelInterface::SUB_REQUEST, $html_response);
    +      $this->eventDispatcher->dispatch(KernelEvents::RESPONSE, $event);
    +      $this->requestStack->pop();
    

    I'm wondering whether we need to try catch here and reset the request stack in case some exceptions are thrown inside.

  5. +++ b/core/modules/big_pipe/src/Render/BigPipeResponseAttachmentsProcessor.php
    @@ -0,0 +1,113 @@
    + */
    +class BigPipeResponseAttachmentsProcessor extends HtmlResponseAttachmentsProcessor {
    ...
    +    $this->htmlResponseAttachmentsProcessor = $html_response_attachments_processor;
    

    So we both decorate and extend the class? Is it just me or is this weird?

Status: Needs review » Needs work

The last submitted patch, 210: big_pipe-2469431-210.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
9.48 KB
1.04 KB
409 bytes
118.09 KB

This fixes the fail in ComposerIntegrationTest. All other failures are a complete mystery to me, and they all are in the much-loved ConfigImportAllTest and InstallUninstallTest tests :P

Status: Needs review » Needs work

The last submitted patch, 214: big_pipe-2469431-214.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
118.2 KB
938 bytes

@dawehner and @alexpott helped me figure out a solution to the bizarre failures in #214. Turns out Alex had already seen this problem in #2342015: Content Translation module still implements hook_enable, the root cause is that big_pipe_page_attachments() is called before the router is rebuilt with the new routes. This is a bug in the module/routing system, which is being fixed at #2589967: Rebuild routes immediately when modules are installed. For now, we can use the same work-around as #2342015: Content Translation module still implements hook_enable used.

Status: Needs review » Needs work

The last submitted patch, 216: big_pipe-2469431-216.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
119.23 KB
5.78 KB

#212:

This is by far the best documented module I've seen. Really great work!

Thanks :) I felt that good documentation for this module is absolutely essential because otherwise it will be unmaintainable a year from now. Note that I did that three months ago, on this issue.

  1. I've also thought about that. I don't think we can. Or at least I don't yet see a way to do this. You would need to be able to use curl or Guzzle at a very low level, where they call a callback as soon as streamed data arrives. AFAIK they only allow us to interact at the response level, we need sub-response level for this.
  2. Good point. Discussed on IRC with @dawehner, changed it to 400.
  3. Good point. Done.
  4. I'm going to add a test case that throws an exception in a #lazy_builder. That's for a future reroll.
  5. I realize this looks a bit bizarre. First and foremost, we decorate, because we want BigPipe's attachments never to make it to \Drupal\Core\Render\HtmlResponseAttachmentsProcessor, because that simply doesn't know how to deal with such attachments.
    Secondly, know that placeholders are bubbled as attachments just like any other attachment (asset libraries, HTML head tags, headers, etc.). But this is the attachment processor. So we have to process all attachments, including placeholders. Placeholders have been already processed by BigPipeStrategy. Placeholders when rendered will not be replaced with the final rendered HTML markup (which would be the case without BigPipe) but instead be replaced with BigPipe placeholder markup, and bubble big_pipe_placeholders + big_pipe_nojs_placeholders. But, if BigPipeResponseAttachmentsProcessor doesn't do that, then HtmlResponseAttachmentsProcessor will, and it will fail miserably because it doesn't know how to deal with big_pipe_placeholders + big_pipe_nojs_placeholders. Therefore, it must happen in BigPipeResponseAttachmentsProcessor.
    Fine, that's a lot of back story, Wim, but what the hell does it have to do with the actual question?
    Well, doing this would duplicate all of \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::renderPlaceholders()! So, rather than duplicating that method, I chose to let BigPipeResponseAttachmentsProcessor extend HtmlResponseAttachmentsProcessor so I can just inherit that method and call it. If you want, I can not subclass it and instead duplicate it. Or perhaps a trait? But then what would be the name of this trait?

Status: Needs review » Needs work

The last submitted patch, 218: big_pipe-2469431-218.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
119.29 KB
1.25 KB

It looks like the other problem lies in the fact that we are in batch (it at least makes config import all tests pass locally).

The last submitted patch, 220: big_pipe-2469431-220.patch, failed testing.

effulgentsia’s picture

The remaining failure is interesting:

fail: [Other] Line 133 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Modules status has been updated.

fail: [Other] Line 139 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
"hook_modules_installed fired for big_pipe" found

When I run the test locally, attached is what I get for my verbose output. Note that I renamed the files from a .html extension to a .txt extension, so that d.o. would accept the upload, so to view them as html, you might need to download them locally and rename back.

The "209-2" file shows the screen following the enabling of experimental_test.module, and note there's a status message for the module being enabled and hook_modules_installed() firing.

However, the "61-2" file shows the screen following the enabling of big_pipe.module, and note there is not a status message for the module being enabled and hook_modules_installed() firing.

I tried to see if I could replicate this outside of the test runner, and I have not yet been able to. I tried adding:

function system_modules_installed($modules) {
  foreach ($modules as $module) {
    drupal_set_message(t('hook_modules_installed fired for @module', array('@module' => $module)));
  }
}

to system.module, and then disabling JS in my browser (to replicate that JS is disabled in the test runner's cURL calls), but even then, when I enable big_pipe.module, I do get the status message appearing to tell me the module was enabled and that hook_modules_installed() fired.

I don't know when I'll next have a chance to keep debugging this, so posting this info in case someone else wants to take it from here.

Wim Leers’s picture

#220: Hah, that makes sense! There were two competing <meta http-equiv="Refresh" …> tags, and BigPipe's was winning. Great find :)


#222: I repeated the debugging steps in #222 (and actually arrived at them independently) and can confirm that the message shows up just fine with that system_modules_installed() implementation.

However, considering the very serious module installing race condition described in #216, I could also imagine there is a race condition here. So I restricted the test to just install the big_pipe module, to make it much faster/easier to debug (20 seconds instead of endless on my computer). The test still fails then. I also set $this->dumpHeaders = TRUE, so we get header debug output. (See dbg-do-not-test.patch.)

If you then re-run the test, you can see there is first the no-JS redirect, and then there is the actual response. Let's first make sure that after the redirect, the BigPipe-no-JS cookie is actually sent to the redirect destination. I added this to big_pipe_page_attachments():

  $page['#attached']['html_head'][] = [
    [
      '#tag' => 'meta',
      '#noscript' => TRUE,
      '#attributes' => [
        'received-big-pipe-no-js-cookie' => $has_big_pipe_nojs_cookie ? 'yes' : 'no',
      ],
    ],
    'debug',
  ];

That results in a yes, so that works. So it's not a bug in curl/WebTestBase.

Second, let's see if it works if we pretend JS is enabled: remove big_pipe_page_attachments() and re-run the test. Now there is no no-JS redirect, and consequently BigPipe assumes JS is enabled. Look at the verbose output with JS enabled, and note that the expected messages now do show up. Disable JS, and they don't show up, because they're streamed as embedded AJAX responses that need JS to be put in the right place, and the test runner doesn't execute JS.
(Note that this time there is still a redirect! I haven't had the time yet to investigate this, but there's a 303 here (also in HEAD), whereas with BigPipe there's a 302 (the BigPipe no-JS redirect). So with BigPipe patch: 302 -> 200, without the no-JS redirect: 303 -> 200.)

So, we now know that this can only be reproduced when JS is off, that the no-JS redirect works correctly (i.e. the no-JS cookie is sent with the request) also in the test runner, but that somehow it still managed to not work inside the test, but do work when used manually. Which makes me suspect there's a bug in the no-JS support of BigPipe. But why is that only a problem in this particular case?

So, let's add debug output to the no-JS rendering of BigPipe. (See dbg-big_pipe_nojs-do-not-test.patch.)

<div class="region region-content">
    [RENDERING BIG PIPE NO-JS PLACEHOLDER "&lt;div data-big-pipe-nojs-placeholder-id=&quot;callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&amp;amp;args[0]&amp;amp;token=a8c34b5e&quot;&gt;&lt;/div&gt;"][RENDERED OUTPUT FOR NO-JS PLACEHOLDER "&lt;div data-big-pipe-nojs-placeholder-id=&quot;callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&amp;amp;args[0]&amp;amp;token=a8c34b5e&quot;&gt;&lt;/div&gt;"="<link rel="stylesheet" href="/core/themes/classy/css/components/messages.css?o379go" media="all" />

"]<link rel="stylesheet" href="/core/themes/classy/css/components/messages.css?o379go" media="all" />


  <h1 class="page-title">Extend</h1>

So, as you can see, the placeholder for status messages does get the expected associated CSS bubbled. So it does get rendered correctly. It's just that there literally is no message to be printed. Let's verify that by doing var_dump($_SESSION['messages']); in the logic to send no-JS placeholders. Doing that yields:

Notice: Undefined index: messages in Drupal\big_pipe\Render\BigPipe->sendNoJsPlaceholders() 

So it literally doesn't exist!

… does this mean that Curl/WebTestBase is somehow perhaps no longer sending a session cookie, which would lose the session to effectively be lost after the redirect? Let's check that by adding something similar to what we added before:

  $page['#attached']['html_head'][] = [
    [
      '#tag' => 'meta',
      '#noscript' => TRUE,
      '#attributes' => [
        'session-exists' => $session_exists ? 'yes' : 'no',
      ],
    ],
    'debug2',
  ];

That results in a yes, so that works. So it's not a bug in curl/WebTestBase.

Tentative conclusion: the problem must lie in where/how $_SESSION['messages'] gets populated.

It might be related to that 303 that happens after installing a module that BigPipe causes to change to disappear, because it's no-JS 302 redirect appears to somehow override it? That's the only thing I didn't investigate above. However, the 303 is just regular Form API business: \Drupal\Core\Form\FormSubmitter::redirectForm() does return new RedirectResponse($url, Response::HTTP_SEE_OTHER);. I don't see how that could interfere with the message.

All help welcome. I'm out of ideas.

swentel’s picture

fail: [Other] Line 139 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
"hook_modules_installed fired for big_pipe" found

This one is easy: as I've identified already, the meta refresh eats the message. Ignoring 'system.modules_list' makes this work too. Which makes me wonder whether we should make this configurable, through config or settings, or some info hook, because maybe other routes in contrib or custom may want to exclude themselves from this part of the code of bigpipe as well ?

fail: [Other] Line 133 of core/modules/system/src/Tests/Module/InstallUninstallTest.php:
Modules status has been updated.

This one still failed after ignoring that route. The problem seems that assertText() in simplest is not able to parse text that is inserted via javascript which contains tags (in this case the <em class="placeholder">. It works for the other other message though which doesn't have tags in it (see screenshot underneath) Haven't looked deeply in detail though, but I added a special check in the test looking for the raw data that is in the javascript and then it's fine.

Wim Leers’s picture

the meta refresh eats the message.

Why? Oh… is it simply that the original page actually contained the message, but then a redirect happens? That'd totally make sense…

The problem seems that assertText() in simplest is not able to parse text that is inserted via javascript

No, the problem is that without the meta refresh, BigPipe assumes JS is present, and hence uses the JS-powered BigPipe streaming of responses, which then indeed causes it not to find this text because it's encoded in a different (embedded JSON) way.

But the no-JS redirect would already exist as soon as you access /admin/modules, so then the no-JS cookie would already be set. Is that test somehow not accessing /admin/modules first, before installing a module?

(I can't check, I'm not at my work computer.)

swentel’s picture

Hmm, not following completely, and can't really answer in detail, but it made me think again and understand this now:

This assert

$this->assertText(t('hook_modules_installed fired for @module')

actually finds the text in the javascript output at the bottom, and the other not because of the tags. Wow, that is an interesting fact :)
So my screenshot is a bit misleading because in the output the javascript is actually doing it's job and replacing the placeholder I guess ?

- edit -

So some conclusion: the refresh ate the messages. On ignoring the route, we were 'lucky' that assertText could find the 'modules_installed' text because it doesn't contain tags. Wow, this is one lucky debugging really :)

The last submitted patch, 224: big_pipe-2469431-223.patch, failed testing.

Wim Leers’s picture

#226: Yup, exactly :)


Thinking about it again, I now fully understand the problem.

First: This is a problem only when installing the BigPipe module through the UI with JS turned off. (And the problem is limited to not seeing this single status message.)

Second: the problem is this: upon installing BigPipe via POST /admin/modules, the response redirects you to GET /admin/modules (both with and without this patch). With BigPipe active, the response to that GET request uses BigPipe for delivery. So, the status message is actually a BigPipe placeholder, and it gets delivered as a BigPipe placeholder: through embedded JSON that is streamed. But! At the top of this page, there is also the no-JS redirect (which #224 removes). So, that redirect is followed… and the same page is loaded with BigPipe delivering the page without the need for JS (i.e. streaming the content until a first no-JS placeholder, then rendering that, then the content until the next placeholder, etc.). This works fine too. But now there is no status message! Why? Simple: because the status messages have already been delivered by that initial response!

So this is an absolutely crazy edge case that happens only for the user installing BigPipe for the first time, if and only if that user installs BigPipe using the UI, and has JS disabled in their browser.

swentel’s picture

Cool - looking at why it still fails though, it works on my local machine with this extra debugging before the while loop starts:

    $debug['color'] = $all_modules['color'];
    $debug['big_pipe'] = $all_modules['big_pipe'];
    $debug['syslog'] = $all_modules['syslog'];
    $all_modules = $debug;

So, I'm expecting that is the problem, digging.

Wim Leers’s picture

So I implemented my analysis of #228.

Interdiff relative to #218. Also committed to the BigPipe contrib module: http://cgit.drupalcode.org/big_pipe/commit/?id=072b7bb (and that matches the interdiff).

THIS SHOULD BE GREEN!

Many thanks to @swentel for all his help!

swentel’s picture

+++ b/src/Render/Placeholder/BigPipeStrategy.php
@@ -83,22 +84,41 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {
    * @param \Drupal\Core\Session\SessionConfigurationInterface $session_configuration
...
+    // Don't apply the BigPipe placeholder strategy on the module installation
+    // page. When a user would install the BigPipe module using a browser that
+    // has JavaScript disabled, the status messages shown immediately after
+    // installing the module would get lost.
+    // @see https://www.drupal.org/node/2469431#comment-10901944
+    if ($this->routeMatch->getRouteName() === 'system.modules_list') {
+      return [];
+    }

Oh yeah, that's a cleaner strategy than I was going for (special casing in simpletest, and my patch in #224 actually missed that interdiff anyway, but it wan't the best solution)

Wim Leers’s picture

FileSize
125.12 KB
1.45 KB

Gah, a unit test will fail in #230, I forgot to update it.

The last submitted patch, 230: big_pipe-2469431-230.patch, failed testing.

swentel’s picture

The code itself looks fine, just pasting some minor things here.

  1. +++ b/core/composer.json
    --- /dev/null
    +++ b/core/modules/big_pipe/README.md
    

    So, this is the second module in core having a README file. Inline form errors has one as well (without the extension though). Should we make the style a bit the same like that one ?

    Scratch that first sentence, I was looking at the wrong file. But the rest underneath still stands though.

    I'm even wondering how useful it is anyway as I'm guessing the documentation and links will be the same on d.o as well ? And if not, the installation part seems a little, well redundant I think no ?

  2. +++ b/core/modules/big_pipe/big_pipe.module
    @@ -0,0 +1,74 @@
    +  // Exclude a few very special routes from no-JS detection.
    +  $excluded_routes = [
    +    // The batch system already uses a <meta> refresh to work with JS disabled.
    

    Like I already suggested, should we make this potentially overridable via settings ? Can't think of any other edge cases in core right now, but maybe there are in contrib or for custom code ?

  3. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -0,0 +1,277 @@
    +    if ($this->routeMatch->getRouteName() === 'system.modules_list') {
    +      return [];
    +    }
    

    Maybe we can then re-use the same settings here ?
    It seems equally unnecessary to use placeholdering for batch too.

chx’s picture

I will not pretend I can understand this so I will just nitpick. +- This results in hugely improved front-end/perceived performance (watch the 40-second on the project page) -- no longer a separate project. That assert set equals is slightly odd and slightly wasteful count($a) == count($b) && !array_diff($a, $b) should be enough (as long as $a and $b are both finite :P ) but meh it's a test.

You left yiha.patch in the patch. I can't see why it's necessary. Also, I believe the spelling of that is yee-haw :D

+ flush(); -- are we use this is enough? I remember vendor/symfony/http-foundation/Response.php having some crazy code to flush. It was something to do with output buffers so I am not sure it's relevant.

Altogether this is insanely exciting and having written https://www.drupal.org/project/redis_ssi I am really excited to see what we can do with this.

Wim Leers’s picture

#234

  1. I'm fine with removing this README and instead having all that on the d.o handbook page. But IIRC Moshe preferred it to have this in the module itself. Whatever the community/committers want.
  2. Let's introduce a no_big_pipe route option. Then there is no need for settings, just a route option to specifically indicate that a particular route does not want BigPipe. That'd be similar to https://www.drupal.org/node/2463533/[#2461087], so it'd just follow an existing pattern. I should've thought of that when I rolled #230 in the first place.
  3. See prior point.

#235:

  1. RE: assertSetEquals(): nice, will do that :)
  2. RE: yiha.patch: LOL OOPS. I rolled that just before having to jump in the car. That also explains the mysterious patch size increase that I couldn't explain. Will fix.
  3. RE: flush() are we using this enough? → I'm not quite sure how to interpret this. If you're asking if the BigPipe module is using this enough: yes, absolutely. If you're asking whether Drupal at large is using this enough: definitely not. The craziness in Symfony's Response.php is for PHP's own output buffering, which requires the use of ob_start() to start such a buffer, which Drupal nor Symfony do anywhere.
  4. Altogether this is insanely exciting

    :)

Will reroll with all this tomorrow.

Anonymous’s picture

Isn't no_cache sufficient, do we need another flag(no_big_pipe)?

chx’s picture

I had no problems with the README itself but it refers to a video on the project page and now that this is in core, that makes no sense.

I mean that flush() might not work by itself and you might need to flush all output buffers first -- if there are any. Not sure. But php.ini has output_handler so there can be output buffering going on even if you didn't start. I might be overthinking this.

chx’s picture

Ps. I meant array_diff_assoc() , otherwise keys are not compared.

dawehner’s picture

Super nitpick: Afaik the technology is called BigPipe as a name and not big pipe, so maybe we could name the module 'bigpipe' instead.

Wim Leers’s picture

#237: Something that is not cacheable can still be served via BigPipe. They're two very different things. So no.

#238: That was actually in the code but it was removed at some point because it was found to be unnecessary. I can add it back to be 100% sure though.

#240: But CamelCase is camel_case when converted to underscores.

Wim Leers’s picture

This cleans up the approach @swentel and I used in #218–#230, in favor of a new _no_big_pipe route option that allows routes to opt out from BigPipe HTML delivery. With test coverage.

This then addresses the vast majority of the feedback. I'll address the remaining nits next.

(Also committed to the contrib module: http://cgit.drupalcode.org/big_pipe/commit/?id=1a209b7)

Status: Needs review » Needs work

The last submitted patch, 242: big_pipe-2469431-242.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
484 bytes
119.36 KB
5.4 KB

Done:

  1. #234/#238: removed the README. I made sure all documentation in it now lives at https://www.drupal.org/documentation/modules/big_pipe + https://www.drupal.org/documentation/modules/big_pipe/environment
  2. #235/#239: assertSetEquals() (also committed to contrib module: http://cgit.drupalcode.org/big_pipe/commit/?id=517f4ab)
  3. #235: the accidental yiha.patch
  4. #235/#238: RE: PHP's output handling & the need for ob_end_flush(): I'd rather not have it in there for now until we run into a clear use case that we can then write test coverage for. This is the perfect sort of edge case that we can flush out (pun intended) while this module is experimental.

The only bit of feedback that has not yet been addressed yet is then @dawehner's #212.4. I'll try to get it done today, but I think everybody would agree that can easily be done in a follow-up; that that should not block RTBC/commit.

Wim Leers’s picture

FileSize
119.47 KB
938 bytes

I reverted #216's changes in #242 and should not have.

tstoeckler’s picture

The craziness in Symfony's Response.php is for PHP's own output buffering, which requires the use of ob_start() to start such a buffer, which Drupal nor Symfony do anywhere.

I think the following tangential to this issue, but I wanted to mention it nonetheless, because it makes the above (arguably?) incorrect. Hope it's not completely off-topic. \Twig_Template::render() is:

    public function render(array $context)
    {
        $level = ob_get_level();
        ob_start();
        try {
            $this->display($context);
        } catch (Exception $e) {
            while (ob_get_level() > $level) {
                ob_end_clean();
            }

            throw $e;
        }

        return ob_get_clean();
    }
borisson_’s picture

I found a couple of things in #242 that I think can be improved, they are very nitpicky though, so it shouldn't hold back getting this in, those can be fixed in a followup.

  1. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
    @@ -0,0 +1,162 @@
    +    // Create a new BigPipeResponse.
    +    $big_pipe_response = new BigPipeResponse();
    +    $big_pipe_response->setBigPipeService($this->bigPipe);
    

    This comment doesn't really help.

  2. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,432 @@
    +      // @todo What if drupalSettings already was printed in the HTML <head>? That case is not yet handled. In that case, no-JS BigPipe would cause broken (incomplete) drupalSettings… This would not matter if it were only used if JS is not enabled, but that's not the only use case. However, this
    +      $cumulative_assets->setAlreadyLoadedLibraries(array_merge($cumulative_assets->getAlreadyLoadedLibraries(), $html_response->getAttachments()['library']));
    +      $cumulative_assets->setSettings($html_response->getAttachments()['drupalSettings']);
    

    We should add a d.o issue we can link to, or at least wrap this within 80 cols and finish the sentence.

  3. +++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
    @@ -0,0 +1,50 @@
    +  }
    +}
    

    There should be a newline before the class closing bracket.

  4. +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    @@ -0,0 +1,338 @@
    + * Provides BigPipe placeholder test cases for use in both unit tests and
    + * integration tests.
    

    The description should be only one line.

  5. +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    @@ -0,0 +1,338 @@
    + * - Unit test: \Drupal\Tests\big_pipe\Unit\Render\Placeholder\BigPipeStrategyTest
    + * - Integration test for BigPipe with JS on: \Drupal\big_pipe\Tests\BigPipeTest::testBigPipe()
    + * - Integration test for BigPipe with JS off: \Drupal\big_pipe\Tests\BigPipeTest::testBigPipeNoJs()
    

    This should be rewrapped for 80cols.

  6. +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    @@ -0,0 +1,338 @@
    +class BigPipePlaceholderTestCase {
    

    Several of the variables here should need newlines between the variable and the docblock.

  7. +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    @@ -0,0 +1,338 @@
    +  /**
    +   * The expected BigPipe no-JS placeholder and corresponding render array and
    +   * embedded HTML response.
    

    Should be only one line.

  8. +++ b/core/modules/big_pipe/src/Tests/BigPipePlaceholderTestCases.php
    @@ -0,0 +1,338 @@
    +  function __construct(array $render_array, $placeholder, array $placeholder_render_array) {
    

    This needs the be public function

  9. +++ b/core/modules/big_pipe/src/Tests/BigPipeTest.php
    @@ -0,0 +1,357 @@
    +      $cases['edge_case__invalid_html']->bigPipeNoJsPlaceholder     => $cases['edge_case__invalid_html']->embeddedHtmlResponse,
    +      $cases['html_attribute_value']->bigPipeNoJsPlaceholder        => $cases['html_attribute_value']->embeddedHtmlResponse,
    +      $cases['html_attribute_value_subset']->bigPipeNoJsPlaceholder => $cases['html_attribute_value_subset']->embeddedHtmlResponse,
    ...
    +      $cases['html']->bigPipePlaceholderId                             => Json::encode($cases['html']->embeddedAjaxResponseCommands),
    +      $cases['edge_case__html_non_lazy_builder']->bigPipePlaceholderId => Json::encode($cases['edge_case__html_non_lazy_builder']->embeddedAjaxResponseCommands),
    

    The arrays here shouldn't be aligned like that.

  10. +++ b/core/modules/big_pipe/src/Tests/BigPipeTest.php
    @@ -0,0 +1,357 @@
    +      $cases['edge_case__invalid_html']->bigPipeNoJsPlaceholder           => $cases['edge_case__invalid_html']->embeddedHtmlResponse,
    +      $cases['html_attribute_value']->bigPipeNoJsPlaceholder              => $cases['html_attribute_value']->embeddedHtmlResponse,
    +      $cases['html_attribute_value_subset']->bigPipeNoJsPlaceholder       => $cases['html_attribute_value_subset']->embeddedHtmlResponse,
    +      $cases['html']->bigPipeNoJsPlaceholder                              => $cases['html']->embeddedHtmlResponse,
    +      $cases['edge_case__html_non_lazy_builder']->bigPipeNoJsPlaceholder  => $cases['edge_case__html_non_lazy_builder']->embeddedHtmlResponse,
    

    See above.

  11. +++ b/core/modules/big_pipe/src/Tests/BigPipeTest.php
    @@ -0,0 +1,357 @@
    +  protected function getTestCases() {
    

    I think this method needs a docblock as well.

Wim Leers’s picture

Thanks, will address all of those.

Wim Leers’s picture

#246: I should have phrased that differently. Grep all of Drupal 8 for ob_start(), and you'll indeed find occurrences (in the Diff component, in Twig like you've shown, in batch, in SystemInfoController::php() to show phpinfo()'s output), et cetera. However, in every single one of those cases, there is a guaranteed closing of the output buffer. Every single one of those cases is about capturing the output of something.
Which means that none of those cases can ever affect BigPipe, and BigPipe doesn't need to call ob_end_flush().

And if we happen to find a case eventually where ob_end_flush() is necessary, then I repeat the answer in #244.4:

RE: PHP's output handling & the need for ob_end_flush(): I'd rather not have it in there for now until we run into a clear use case that we can then write test coverage for. This is the perfect sort of edge case that we can flush out (pun intended) while this module is experimental.

tstoeckler’s picture

Right, I had assumed something along those lines (but it wasn't as clear in my head). Thanks! And sorry for the derailing...

Wim Leers’s picture

FileSize
119.36 KB
5.44 KB

#247: all addressed. Disagreed with points 9 and 10, because that alignment is significantly better for legibility in this case.

dawehner’s picture

+++ b/src/Render/BigPipe.php
@@ -264,7 +264,6 @@ class BigPipe implements BigPipeInterface {
-      // @todo What if drupalSettings already was printed in the HTML <head>? That case is not yet handled. In that case, no-JS BigPipe would cause broken (incomplete) drupalSettings… This would not matter if it were only used if JS is not enabled, but that's not the only use case. However, this

Wait, so we drop @todos ? Wasn't that a valid point?

Wim Leers’s picture

Issue summary: View changes

Updated the Remaining tasks section to be up-to-date.

#252: It wasn't a valid point AFAICT. Note that I wrote that myself as a future todo for myself. The fact that I never finished the sentence also indicates that I failed to see how it could actually break something, even in an extreme edge case. That todo should never have been committed.

#212.4 is the only remaining task. I'll try to get it done today, but IMO it shouldn't block this getting RTBC'd/committed, it's pure hardening that can easily happen in a follow-up.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this then!

Fabianx’s picture

RTBC + 1, I reviewed all patches since my own work on this and while I obviously can't RTBC the whole patch, I can RTBC the interdiffs and am doing so.

I think we could add a call to:

Symfony\http_foundation\Response::flushOutputBuffers() in the future, but I don't think it is necessary, yet. In the worst case someone could add a subscriber to stop output buffers if it ever becomes a problem in an environment, so the module would not need to be changed to fix this.

Regarding the removed @todo: I agree that it is a rare edge case and should be checked in a follow-up.

nod_’s picture

JS is good to go.

dawehner’s picture

should be checked in a follow-up.

Well yeah, this is what I was arguing for, but sure, let's ignore that.

effulgentsia’s picture

This patch looks pretty fantastic to me. Here are some things that I think can be post-beta follow-ups rather than block commit.

  1. +++ b/core/modules/big_pipe/js/big_pipe.js
    @@ -0,0 +1,107 @@
    +      // @see bigPipeProcessContainer()
    

    References a function that doesn't exist.

  2. +++ b/core/modules/big_pipe/js/big_pipe.js
    @@ -0,0 +1,107 @@
    +        ajaxObject.success(response);
    

    Should we also pass a status argument, in case there are contrib commands that expect it? Note that Drupal.Ajax.prototype.success() is documented as status being a numeric code, but I don't think that's correct, since it's normally passed from ajax.options.success(), which jQuery documents as being textStatus rather than statusCode.

  3. +++ b/core/modules/big_pipe/js/big_pipe.js
    @@ -0,0 +1,107 @@
    +   *   The HTML document containing <script type="application/vnd.drupal-ajax">
    +   *   tags generated by BigPipe.
    

    The script tags generated by Drupal\big_pipe\Render\BigPipe use application/json as the type.

  4. +++ b/core/modules/big_pipe/src/Controller/BigPipeController.php
    @@ -0,0 +1,64 @@
    +    $response->headers->setCookie(new Cookie(BigPipeStrategy::NOJS_COOKIE, TRUE));
    

    While testing, I switched my browser back and forth from JS enabled to disabled and back. Once this cookie is set, nothing unsets it once JS is re-enabled. Should we add some JS code to do so?

  5. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,431 @@
    +use Drupal\Core\Render\Markup;
    

    Unused.

  6. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,431 @@
    +    list($pre_body, $post_body) = explode('</body>', $content, 2);
    

    This can catch an earlier occurrence of '</body>' than the one we want. For example, such a string within a CDATA section or a JS string value within inline JS. I didn't check if other places where we do similar string parsing is similarly brittle.

  7. +++ b/core/modules/big_pipe/src/Render/BigPipe.php
    @@ -0,0 +1,431 @@
    +      $fake_request = $this->requestStack->getMasterRequest()->duplicate();
    +      $this->requestStack->push($fake_request);
    +      $event = new FilterResponseEvent($this->httpKernel, $fake_request, HttpKernelInterface::SUB_REQUEST, $html_response);
    +      $this->eventDispatcher->dispatch(KernelEvents::RESPONSE, $event);
    +      $this->requestStack->pop();
    

    This pattern is repeated 3 times within this file. Perhaps a protected helper method would be good? I realize the 3 usages aren't identical, so not entirely trivial.

  8. +++ b/core/modules/big_pipe/src/Render/BigPipeMarkup.php
    @@ -0,0 +1,28 @@
    + * Defines an object that passes safe strings through the Views render system.
    

    What does this have to do with Views?

  9. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -0,0 +1,273 @@
    + * The BigPipe placeholder strategy actually consists of two substrategies,
    + * depending on whether the current session is in a browser with JavaScript
    + * enabled or not:
    + * 1. with JavaScript enabled: #attached[big_pipe_js_placeholders]. Their
    ...
    + * 2. with JavaScript disabled: #attached[big_pipe_nojs_placeholders]. Their
    

    I was confused by this doc, because big_pipe_nojs_placeholders can also be used when JS is enabled. The docs following this paragraph allude to that, but I still think this paragraph can be improved to not imply the 1:1 relationship to begin with.

  10. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -0,0 +1,273 @@
    +    // Generate a BigPipe placeholder ID (to be used by BigPipe's JavaScript).
    +    // @see \Drupal\Core\Render\PlaceholderGenerator::createPlaceholder()
    +    if (isset($placeholder_render_array['#lazy_builder'])) {
    ...
    +      return UrlHelper::buildQuery(['callback' => $callback, 'args' => $arguments, 'token' => $token]);
    +    }
    +    // When the placeholder's render array is not using a #lazy_builder,
    +    // anything could be in there: only #lazy_builder has a strict contract that
    +    // allows us to create a more sane selector. Therefore, simply the original
    +    // placeholder into a usable placeholder ID, at the cost of it being obtuse.
    +    else {
    +      return Html::getId($original_placeholder);
    +    }
    

    So here we have a strategy that outputs the placeholder to the client. As opposed to previously in HEAD, where placeholder markup was assumed to only live on the server, such as within render cache entries. It seems not ideal to me for someone on a production site, with no special permissions, to be able to "view source" and see the PHP callback and arguments of the lazy builder, or potentially similar server-side-specific information that might be in $original_placeholder.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 251: big_pipe-2469431-250.patch, failed testing.

catch’s picture

Haven't managed to review the whole patch yet, although what I read so far was encouraging.

One docs issue I noticed:

  1. +++ b/core/modules/big_pipe/src/Render/BigPipeInterface.php
    @@ -0,0 +1,131 @@
    + * 2. No-JS BigPipe placeholders: 1 HtmlResponse + N embedded HtmlResponses.
    + *   - See first bullet of point 1.
    + *   - No-JS BigPipe does not use multiple AJAX requests/responses. It uses a
    + *     single HTML response. But it is a more long-lived one: The Skeleton is
    + *     split into multiple parts, the separators are where the no-JS BigPipe
    + *     placeholders used to be. Whenever another no-JS BigPipe placeholder is
    + *     rendered, Drupal sends (and so actually appends to the already-sent HTML)
    + *     something like
    + *     <link rel="stylesheet" …><script …><content>.
    + *   - So, for every no-JS BigPipe placeholder, we send its associated CSS and
    + *     header JS that has not already been sent (the bottom JS is not yet sent,
    + *     so we can accumulate all of it and send it together at the end). This
    

    The no-js placeholders aren't like this.

  2. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
    @@ -0,0 +1,273 @@
    +
    +    return [
    +      '#markup' => '<div data-big-pipe-placeholder-id="' . Html::escape($big_pipe_placeholder_id) . '"></div>',
    +      '#cache' => [
    +        'max-age' => 0,
    +        'contexts' => [
    +          'session.exists',
    +        ],
    

    They're like this.

Wim Leers’s picture

Wim Leers’s picture

#258:

  1. Fixed.
  2. Fixed.
  3. Fixed.
  4. Nice feature request. Follow-up created: #2678628: Consider removing the BigPipe no-JS cookie when JS is enabled again in a browser
  5. Fixed (leftover).
  6. Good point, that'd be good hardening to do. Follow-up created: #2678662: Ensure BigPipe does not break when HTML document contains CDATA sections or inline scripts matching certain patterns.
  7. Good idea, done!
  8. Fixed (was copy/paste remnant).
  9. But that 1:1 relationship *is* the general rule. So I think the existing docs are sufficiently clear: they first explain the usual 1:1 relationship, then explain the edge case. If you have concrete doc improvement suggestions, we can discuss those in a follow-up.
  10. I don't see how this more harmful than disclosing entity and field identifiers through Quick Edit's data- attributes, field access information through Quick Edit's metadata, cache tags or cache contexts. Only primitives (string/integer/boolean/null) are accepted values, which means (and the whole point is) that you can only pass identifiers of things as #lazy_builder arguments. Those identifiers can 99% of the time be looked up by looking at a Drupal code checkout.
    Nevertheless, I've opened an issue to discuss this further: #2678658: Consider omitting lazy builder callback + arguments from the BigPipe placeholder ID.

#259: (i.e. failed patch): WTF!? Is this a random failure? #251 used to be green, now it's red. Was this caused by recent changes?

Interdiff also committed to the contrib module: http://cgit.drupalcode.org/big_pipe/commit/?id=a4c3d4e

dawehner’s picture

Re #255 Well, the point I tried to make was that a non issue should be documented in a follow up, and not just ignored, this is all.

Wim Leers’s picture

#260: Discussed with catch in IRC to understand what he meant; his point was that one step/phase was missing in the BigPipeInterface docs: the phase where BigPipe (no-JS or otherwise) placeholder markup exists. Now documented. He also pointed out that further down, where it says placeholder, it should actually say placeholder replacement. Addressed that also.

Wim Leers’s picture

Gábor Hojtsy’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#262 and #263 are green. The sudden bizarre random failure of #251 has an issue to further investigate that (thanks @Gábor Hojtsy!): #2678770: Random fail with third party shortcut settings schema on seven theme.

#262 and #263 only addressed nitpicks, so back to RTBC per #254.

effulgentsia’s picture

I think this has been adequately reviewed at this point, and no commit-blocking concerns have been raised. Thanks for creating the follow-up issues for non-commit-blocking feedback. I'm about to commit this.

First, ticking credit boxes for reviewers who performed code reviews on this issue. Apologies if I missed adding credit for other substantive contribution.

  • effulgentsia committed dda8487 on 8.1.x
    Issue #2469431 by Wim Leers, Fabianx, swentel, nod_, yched, dawehner,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.1.x. Thanks, everyone! Great work!

Wim Leers’s picture

Component: request processing system » big_pipe.module

Moving to its own component.

xjm’s picture

Issue tags: +8.1.0 release notes

Status: Fixed » Closed (fixed)

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

RavindraSingh’s picture

I am not sure if I should re-open this issue. I tried to degrade the Drupal 8 instance but still not able to enable the module.

Drupal BigPipe

colan’s picture

@RavindraSingh: Please open a new issue for that (unless one exists already).

ar-jan’s picture

#274: you have the contrib module big_pipe in your modules directory. If you remove it, the bigpipe module included in Drupal 8.1 will be listed (under Experimental modules).