Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits

Disqus is loaded via JS. No AJAX (or at least not to Drupal, perhaps to an external service, but then we don't need to care about that).

Note that we already designed BigPipe to make this possible. Quoting \Drupal\big_pipe\Render\Placeholder\BigPipeStrategy:

 * First: the BigPipe placeholder strategy only activates if the current request
 * is associated with a session. Without a session, it is assumed this response
 * is not actually dynamic: if none of the placeholders show session-dependent
 * information, then none of the placeholders are uncacheable or poorly
 * cacheable, which means the Page Cache (for anonymous users) can deal with it.
 * In other words: BigPipe works for all authenticated users and for anonymous
 * users that have a session (typical example: a shopping cart).
 *
 * (This is the default, and other modules can subclass this placeholder
 * strategy to have different rules for enabling BigPipe.)

This issue is basically asking to make sessionless BigPipe a configuration option. Assigning to Fabianx for feedback.

Looking up configuration costs valuable time. This is also why the Page Cache is no longer a checkbox on that same performance settings page you mentioned, but a module without configuration. You enable the module, it has zero configuration. BigPipe is similar.

BigPipe in core implements a very simple session-based heuristic: session-less content first, session-full content second.

The fact that some session-less content is actually more expensive than some session-full content in a particular use-case is not BigPipe's concern. If we want to implement some sort of hybrid, we can extend (or not) BigPipe and swap the render strategy using contrib.

Re-opening.

There is 2 cases where session less BigPipe is useful and could be combined with PageCache (that in theory can cache big pipe responses):

• 1. Deliberate spam protection, a side effect, but also interesting for blocks that take a long while to load - even if they could be cached.
• 2. First time impression for uncached pages, in which case BigPipe would store the assembled page in page_cache as if it was never BigPipe'd in the first place.

Both cases contradict each other (e.g. 1 explicitly wants to cache a BigPipe response, while 2) wants to cache a normal response from the fragments rendered by BigPipe).

1. could be implemented pretty easily by forcing a big_pipe placeholder via #placeholder_options and taking that into account, e.g. the session only mode is only for auto-placeholdered fragments - not for forced ones.

2. could be implemented if rendered placeholders had a 'static' cache somewhere in which case the page cache after the page is delivered would not need to re-render all fragments, but just re-compose the page.

Putting back to active for now - as there are now use-cases, one of them endorsed by Dries. (the spam protection)

I vehemently disagree with #8.1. It's not at all an effective anti-spam measure. Many spambots actually execute JS. If they don't yet, they can easily do so. Then you're once again at the same place. Dries did not endorse it. He found it interesting. I talked to him about it, and he sees now how he misunderstood it.

However, number 2 is a very interesting possibility, and the strongest reason I've seen so far to make BigPipe sessionless. It's a very compelling feature. And the interesting thing is that after the first response, it will be cached in the PageCache, which means that it effectively would still only be used for responses with sessions, because those requests without sessions will still end up being served by the Page Cache! I love that.
Conversely, if you don't use the Page Cache, every anonymous response can be delivered via BigPipe as well, and when Page Cache is disabled, that's probably because your anonymous responses are highly dynamic too!
This makes perfect sense.

Updating issue title accordingly.

That being said, this could IMO be a new feature for BigPipe for Drupal 8.2. I'd prefer to first stabilize BigPipe. OTOH, this would make BigPipe much more appealing, since it then also improves the experience for sites with only anonymous users. So, +1 for doing this.

I've provided a very, very rough implementation. And it works. The most important remaining problem:

• Ideally: BigPipe responses for users with JS would be converted down so they don't need JS. But that's not trivial. What we could do as an alternative, is letting BigPipe make PageCache aware of the BigPipe no-JS cookie, and let it serve a different response based on that.
• Or … whenever BigPipe is used for anonymous users, never ever rely on JS. i.e. when the user is anonymous, do no-JS BigPipe response delivery. That still means a streamed response. But it removes the need for a redirect, it removes the need for making difficult conversions. Because the end result is still a simple HTML response.

Implementing my second suggestion at the bottom now.

Also, #9 is going to fail many tests. I should have marked it -do-not-test. Test it manually though, you'll see it works fine.

Here you go. Smaller patch too! :D The interdiff may be confusing because it restores so much, just read the patch itself, it's about the same size anyway.

This makes responses for anonymous users streamed responses, which are delivered much faster. Subsequent responses are just delivered by the Page Cache (are not streamed). In other words, this makes cold cache hits for anonymous users significantly less painful.

Still to do:

1. cacheability metadata: tags + max-age (expiration date)
2. clean-up
3. tests

#9: Pardon, I had misunderstood Dries reaction there. And yes, they could activate Javascript. However JS parsing costs time and waiting for a fragment to occur costs, too. So deliberate slow down can indeed fight spam.

Anyway: +1000 to #13

That is a great start. Nice idea to use NoJSPlaceholders and make the implementation so simple :).

I think it would be best to add an interface to PageCache so that external services like my Pre-Kernel middleware or BigPipe can interface with it properly - instead of copying the functionality.

Once we have the EmitterInterface, it will be even nicer as we can just do:

$response->setEmitter(NULL);
$response->setContent($content);
$response->setAttachments([]);

$page_cache->set($response);

And we are done! As BigPipe then no longer is creating and re-creating a new response, but just transforming its contents and removing the emitter!

So that and that refreshless wants to stream a JsonResponse potentially with a custom emitter would be enough for me to say that we should prioritize EmitterInterface, too.

This seems like a sensible feature to me, but 8.2 is the current branch for new features.

From the issue summary:

I want this block to show different products each time the page is refreshed.

This seems out of scope to me for this issue, and AFAICT, not implemented by #13. I see this issue already has the "Needs issue summary update" tag, which I'm guessing should include changing that portion?

#15: yes, I changed the direction of this issue — I indeed intend to update the IS.

Also note that because BigPipe is experimental, new features can go into 8.1.

Now making this happen! :)

First, a reroll to allow it to apply cleanly to HEAD again. And one change that was overlooked in #13, that allows it to pass tests. Of course, this new functionality itself still needs tests, but the fact that it's green despite the extensive existing test coverage proves that this slots in cleanly. :)

Also, because this is an experimental module, this new feature can actually go into Drupal 8.1 too. So, moving to 8.1.x-dev.

Updated IS, addresses #15.

This fixes the most important failures, which were simply a case of me not yet having updated the expectations in BigPipeStrategyTest. This should bring it down to one failure.

More work to follow, of course.

I wanted to clarify that in fact BigPipe has always supported anonymous users. It just has only supported anonymous users with sessions.

This issue is adding support for the case of anonymous users without sessions.

It's still going to be recommended to use Page Cache/Varnish/… if your responses are not dynamic (i.e. identical for all sessionless anonymous users). However, if your response is dynamic (e.g. customized based on IP address, or changing every request, or changing every second, or …), then BigPipe for sessionless requests can indeed be very appealing, since it brings the ability to stream those responses, which means those responses will render much faster.

And in a default Drupal 8 install, that means we'll be streaming the first response for a sessionless anonymous user. In other words: Page Cache misses will be streamed the first time, and subsequent times they will not be streamed, since Page Cache is much faster, so streaming is pointless (there's nothing expensive to compute anymore).

Was that not yet clear in the IS overhaul I made in #20?

Note that this will mean that https://www.drupal.org/documentation/modules/big_pipe + https://www.drupal.org/documentation/modules/big_pipe/environment will need to be updated. The trigger for streaming should still be the presence of Surrogate-Control: no-store, content="BigPipe/1.0", but it should not disable caching (the beresp.ttl = 0s line should be removed from the example in the docs). BigPipe already appropriately sets Cache-Control: private for requests with sessions. It won't set it for sessionless requests (i.e. this issue). This then signals to Page Cache/Varnish/… that it can be safely cached.

And here is then comprehensive test coverage.

This made me realize one thing that I did not realize before, not even in #24: it's impossible to support cache tags on this. This follows from:

1. An HTTP response must first send its headers, and then its body.
2. For a reverse proxy (like Varnish) to be able to purge a response based on cache tags, the cache tags must be defined in the response headers.
3. But in case of a BigPipe response, the set of cache tags grows as more of the content is streamed, i.e. as more of the placeholders are rendered.
4. Consequently the cache tags header sent before the response body is outdated when the response body has finished sending, but there is no way to still send more headers.

But we of course need responses streamed by BigPipe in case of a Page Cache miss to be invalidated when appropriate when stored in Page Cache. We can ensure this for Page Cache because it introspects the the cacheability metadata stored in \Drupal\Core\Cache\CacheableResponseInterface. That works fine. The only problem is that the design of HTTP prevents us from communicating these cache tags to the outside world.

(In other words: the data dependencies of a streamed BigPipe response are only discovered during streaming, but the use of headers requires all data dependencies to be known ahead of time.)

So, to use BigPipe for sessionless anonymous responses means partially giving up the ability to use tag-based invalidation on Varnish for sessionless anonymous responses. Partially, because: A) all content outside placeholders is still instantly invalidated, B) content inside placeholders is now going to depend on the max-age of the response. Furthermore, it's very rare to see cache tags be used in placeholders, usually the content is not cacheable at all.

All of this is a consequence of this issue allowing BigPipe to be used for cacheable responses — so far BigPipe only supported uncacheable responses, because responses personalized for a particular session can never be cached.

It's at times like these that I miss the old testbot. The output in #25 is highly misleading. There's indeed only one failing test. But the error messages that it shows suggest there's only one real problem. But there's many more:

00:02:27.020 Drupal\big_pipe\Tests\BigPipeTest                            313 passes  12 fails   2 exceptions             

There's twelve failures, yet https://www.drupal.org/pift-ci-job/378101 only shows the two exceptions.

#25: Lets not overcomplicate the issue.

It is enough to not store the BigPipe'd output in Varnish for now.

For cacheable responses, we most likely want the same behavior as with page cache:

• - First request (cache miss) => Big Pipe it and store in Page Cache
• - Second request (cache hit) => Return from page cache

--

For Varnish this changes a little to three requests:

• - First request (page cache cache miss, varnish cache miss) => Big Pipe it and store in Page Cache, but do not store in Varnish
• - Second request (page cache cache hit, varnish cache miss) => Return from page cache, store in Varnish
• - Third request (page cache cache hit, varnish cache hit) => Return from Varnish

For true Varnish BigPipe thingy, you need BigPipe-ESI, which unfortunately Varnish 4 does not support right now (only the commercial version), but Fastly should support it soon.

This patch then brings the first part of the actual solution: removing the need for Page Cache hacks.

First, we need to remember the request/response flow. From index.php:

$response = $kernel->handle($request);
$response->send();
$kernel->terminate($request, $response);

1. That first line is what calls the controller, and goes through all the middlewares.
2. The second line is what actually sends the response.
3. The third line is for doing stuff after the response was sent.

In the case of a non-streamed response, 99% of the work is done on the first line, which means that the PageCache middleware can store the response in its handle() method. But in the case of a streamed response, the bulk of the work is actually done in the second line: that is when placeholders are rendered. Which means that the PageCache middleware cannot store the response in its handle(), because the response still contains placeholders! However, Symfony actually provides a solution for this: TerminableInterface. Middlewares can implement this too.

So the solution is simple:

1. Let BigPipe responses track what has been sent, so that during the termination phase, we can read the entire streamed response.
2. Let the PageCache also work during the termination phase, and just store that streamed response.
3. Profit!

This patch:

1. introduces StreamedResponseInterface and lets BigPipeResponse implement that interface. This interface allows the entire streamed response to be read during the termination phase.
2. changes PageCache to not only ignore BinaryFileResponse + StreamedResponse, but also StreamedResponseInterface
3. the prior point then actually makes the the DenyBigPipeResponse response policy that was introduced in #13 unnecessary, so deleted that
4. lets the PageCache middleware also implement TerminableInterface, and in there, if the response that was just sent implements StreamedResponseInterface, then let it cache the streamed response. This means that some of the logic that lives in PageCache::fetch() in HEAD needs to be shared among PageCache::fetch() and PageCache::terminate() — so that logic has been moved into a protected PageCache::storeResponse() method.

This means we'll need more Page Cache test coverage: we need to test that its TerminableInterface behaves as expected.

The test-only interdiff/patch on this comment brings the updated test coverage. The other interdiff/patch brings the actual changes.

Between #25 and #29, we now have comprehensive test coverage. Updating the IS.

#29: The StreamedResponseInterface is a pretty neat idea.

I wonder if we should not combine it with the 'delivery method' I proposed with Crell and just use setContent() / setCacheabilityMetadata() on the response itself?

Also I feel you want to execute all of that in its own rendering context or do the sub-response fragments take care of that already?

Great work!

#31:

The StreamedResponseInterface is a pretty neat idea.

Glad you like it :) It's the simplest solution possible AFAICT.

I wonder if we should not combine it with the 'delivery method' I proposed with Crell and just use setContent() / setCacheabilityMetadata() on the response itself?

Can you elaborate on this? I don't know what you are referring to.

Also I feel you want to execute all of that in its own rendering context or do the sub-response fragments take care of that already?

First, this remark is out of scope for the current issue.
Second, BigPipe already does that in HEAD, and always has. \Drupal\big_pipe\Render\BigPipe calls \Drupal\Core\Render\Renderer::renderPlaceholder(), which renders in its own render context.

Great work!

Yay! :)

You'll like the next round, which will make this patch green. Stay tuned.

In the mean time, this removes all remaining Page Cache-related hackery from the BigPipe module.

This patch then brings the second part of the actual solution: making BigPipe implement StreamedResponseInterface so that Page Cache can cache it.

This patch:

1. updates BigPipeResponse to make it implement StreamedResponseInterface
2. since implementing this interface requires some logic to live in BigPipeResponse, BigPipeResponse is no longer a super dumb class, consequently it makes sense to move some of the BigPipe header logic from HtmlResponseBigPipeSubscriber.php to BigPipeResponse — even more so because two of the headers (Cache-Control and Surrogate-Control) now depend on whether the BigPipe response is for a request with or without a session
3. since BigPipeResponse must already be aware of it being for a request with/without a session, it makes sense to let this knowledge be passed on to the BigPipe service that it calls for actually streaming the response — so BigPipe(Interface)::sendContent() gained a new parameter
4. cleans up the logic introduced in #13, so that it no longer loses cacheability metadata and attachments. Now uses HtmlResponse objects rather than strings to track the streamed output.

Strange, #36 was not picked up by testbot. Reuploading.

Hah, now there's failures in BigPipeResponseAttachmentsProcessorTest. I didn't run that unit test locally. Will fix those fails.

Note that all other tests pass, so that includes the BigPipe and Page Cache integration tests!

I wanted to discuss that in IRC, but since found my arguments, so writing here:

- As this now changes BigPipeResponse a lot, I would like to implement #2577631: Allow HtmlResponse to use a flexible emitter first, likely the response would still need to be passed from the emitter to the BigPipe service, but I was a fan of that from the start anyway.

The reason being that the emitter or BigPipe service can just call setContent(), setAttachments() and setCacheabilityMetadata() and unset the emitter itself to update the response to the final emitted product and it will just work.

We would not need two new Interfaces, but we could still have them.

Creating a new response and embedding and returning feels strange from an architecture POV.

And Crell was also much in favor of the EmitterInterface and its in Zend Diactoros for PSR-7 (?) https://api.drupal.org/api/drupal/vendor!zendframework!zend-diactoros!sr....

So this feels like the right moment to introduce it.

Adding ResponseEmitterInterface and EmittableResponseInterace would be nice, but … we'd still need a way for retrieving the emitted response.

In other words, you're asking for:

1. add ResponseEmitterInterface (with only a emit() method), and let BigPipe(Interface) implement it
2. add EmittableResponseInterface (with a getter+setter for getting/setting a ResponseEmitterInterface)
3. adding the method that StreamedResponseInterface has (introduced in this patch) to EmittableResponseInterface

Then implementation-wise, that'd mean:

1. ideally reusing it directly from zend-diactoros, but sadly this is impossible because it requires the use of ResponseInterface, which Symfony does not implement
2. this does not exist in zend-diactoros, so we'd roll our own
3. … that we'd be adding something to EmittableResponseInterface that is unnecessary

Point 3 clearly indicates that this is actually completely independent. It's only tangentially related — we'd still need StreamedResponseInterface, because EmittableResponseInterface does not have a way for retrieving the emitted/streamed response.

So you're exactly right at:

We would not need two new Interfaces, but we could still have them.

We could merge them, but then we'd be merging different concerns. Not every streamed/emitted response can (think real-time log data) or should (think streaming of binary files) be retrievable after having been streamed/emitted. So the interfaces are logically different, and must therefore be separate.

The reason being that the emitter or BigPipe service can just call setContent(), setAttachments() and setCacheabilityMetadata() and unset the emitter itself to update the response to the final emitted product and it will just work.

No they can't. Look at BigPipeResponse::getStreamedResponse(). It returns a HtmlResponse object, precisely because it's returning a non-streamed ("fully flushed" if you will) variant of the same response.

I'm strongly doubtful whether this is the right time to introduce response emitters. We need >=2 use cases to prove it's the right abstraction. Drupal 8 has only a single implementation (BigPipe). Yes, it's inspired by Zend's diactoros, but we're adding some of our own stuff on top. Which makes it fairly risky to add this new official API.

I'd much rather do only the one thing that we'll still need to do anyway: add StreamedResponseInterface. We can still do #2577631: Allow HtmlResponse to use a flexible emitter later.

And this then provides the minor changes to BigPipeResponseAttachmentsProcessorTest to allow that test to construct value objects necessary in its testing.

a) StreamedResponseInterface is misleading => Should be CacheableStreamedResponseInterface as the important point is that its cacheable after streaming, not that its a streamed response.

Just some train of thought on emitter vs. non-emitter:

So the control flow currently is:

Partial HtmlResponse => BigPipeResponse + embedded original partial response

The PageCache then checks for an embedded response.

My suggested control flow would be:

- HtmlResponse + some flag that it is uncacheable (e.g. $this->hasEmitter() or $this->isPublic() == FALSE).

- Then HtmlResponse() just gets made cacheable by updating the content

However there is one drawback to the emitter approach:

A Middleware could in theory work with a HtmlResponse using getContent(), but that just returns partial content, while it could detect a BigPipe or CacheableStreamedResponseInterface.

But on the other hand a HTML response can in itself include placeholders, too.

And further getContent() works on the BigPipeResponse as well, so that is not a drawback per se as its similar to check if a response has placeholder attachments (another middleware could take care of that) or check for BigPipeResponse or check for an emitter that this response is using.

--

And we might even think about using "noJS BigPipe by default in Drupal" in the future as there is no drawback.

The only incompatibility comes when a middleware calls getContent() and setContent() and that we could change by ensuring that when getContent() is called the emitter is called with ob_level() > 0. As the emitter also updates the original response, that would mean that the response is then fully build (and was not streamed obviously).

--

All that said, I think using EmitterInterface is cleaner:

- Less copying and cloning around (though we already do that)
- Emitters update the original response as well and make it cacheable again
- PageCache just checks in terminate() again if its cacheable now (I think we might use the private() flag for that). And in terminate() if we do an instanceof check or an instanceof check plus one function call is not that important performance wise.
- We could also instead just always cache in terminate(), which would also very very slightly improve performance as the users get their response faster. (not of any real significance though).
- Then we just need to check once if its cacheable.

The reason why its important to discuss that now is that we introduce here a new interface, but an emitted response just needs to be cacheable after emitting (and reasonably if someone unsets the emitter that would be a good reason to think that), not embed another response in itself.

e.g. there is an inherent conflict in that both approaches have different needs, but we can't break BC after introducing it.

Hence I think its important to get it "right" the first time.

And I do think much more things will use streaming in the future, which emitters allow easily.

Should be CacheableStreamedResponseInterface as the important point is that its cacheable after streaming, not that its a streamed response.

Interesting! This is a superb remark. We should modify StreamedResponseInterface::getStreamedResponse() to not have

@return FALSE|\Symfony\Component\HttpFoundation\Response

but

@return FALSE|\Drupal\Core\Cache\CacheableResponseInterface

So +100.

some flag that it is uncacheable (e.g. $this->hasEmitter()

I'd go with instanceof INTERFACENAME . (See #2577631-33: Allow HtmlResponse to use a flexible emitter, I proposed to not call that EmitterAwareResponseInterface, but StreamedResponseInterface.) Having another method would require it to live on the interface, but it'll just always have to return TRUE then. It's the same thing twice. Either way, you have to check for it: either by calling a method, or checking if an interface is implemented. The latter is DRY, so go with that.

or $this->isPublic() == FALSE).

No! A private response could also be cacheable. Page Cache does this.

However there is one drawback to the emitter approach: […]

I agree that this is not really a drawback. The same caveat/need for carefulness already exists, and is inherent to Symfony's Response design.

And I do think much more things will use streaming in the future, which emitters allow easily.

See how you say "emitters for streaming", this is why I proposed at #2577631-33: Allow HtmlResponse to use a flexible emitter to just call them streamers.

I'm cautiously optimistic that doing #2577631: Allow HtmlResponse to use a flexible emitter actually is the right thing to do.

We discussed this in IRC and the summary is that even with emitters we want CacheableStreamedResponseInterface.

The reason is that we want to run kernel response event listeners again when we have fully assembled the response.

This however we can only do if we stored the original response and cloned it, as response listeners might have changed it before.

Therefore we keep the original response and we need an interface for how to retrieve the original response as Wim is right and those are semantically two different responses.

That means we would have:

- setCacheableStreamedResponse()
- getCacheableStreamedResponse()

and getCacheableStreamedResponse() can also return NULL.

--

What was missing before is that the response again goes via the Kernel Response Event Listeners, but as attachments are empty, only those listeners related to cacheability will run.

--

I would love if HtmlResponse itself would have a copyResponse / duplicate / duplicateFrom / duplicateResponse method.

BigPipeResponse could then just call:

  $this->duplicateFrom($original_html_response).

to init itself instead of cloning from the outside. That would make me more comfortable, but could be a follow-up, too.

First, a straight rebase. #2678662: Ensure BigPipe does not break when HTML document contains CDATA sections or inline scripts matching certain patterns landed recently, and conflicted with this.

#47 was a bad rebase. Copy/pasted a statement to the wrong place.

First, renaming StreamedResponseInterface to CacheableStreamedResponseInterface, updating its @return docblock, marking it @internal with @todo to make it a public API in the future.

Next: the response event subscriber stuff.

One rename was incomplete.

CR created: https://www.drupal.org/node/2779719.

The reason Fabianx and I agreed that this patch should be changed to dispatch the KernelEvents::RESPONSE event is consistency/predictability: this cacheable streamed response should be processed in the exact same way as any other response. And so, also in this case, we need for example FinishResponseSubscriber to run so that the X-Drupal-Cache-Tags header is set correctly, and that means it must include cache tags added by a rendered lazy builder that is streamed to the client. Which means that we must invoke the RESPONSE event again for the cacheable streamed response.

In fact, the new test coverage that this patch is already adding proves this flaw:

      $this->pass('Repeat request: Page Cache hit, BigPipe not involved', 'Debug');
      $this->drupalGet(Url::fromRoute('big_pipe_test'));
      $this->assertIdentical('HIT', $this->drupalGetHeader('X-Drupal-Cache'), 'Page cache hit.');
      $this->assertIdentical($expectations['page cache hit cache control'], $this->drupalGetHeader('Cache-Control'));
      $this->assertFalse($this->drupalGetHeader('Surrogate-Control'), 'No Surrogate-Control header.');
      $this->assertFalse($this->drupalGetHeader('X-Accel-Buffering'), 'No X-Accel-Buffering header.');
      $this->assertNoCacheTag('cache_tag_set_in_lazy_builder');

This verifies that a repeat request is a Page Cache hit, that is not handled by BigPipe. But that last line is asserting that the cache tag set in a #lazy_builder rendered by BigPipe and whose output is streamed, does NOT exist on the cached response. This is wrong! It should exist! (Otherwise external reverse proxies wouldn't be able to invalidate their cached responses based on this cache tag being invalidated, as explained earlier in this comment.)

So, simply changing that to

      $this->assertNoCacheTag('cache_tag_set_in_lazy_builder');

provides the necessary test coverage.

AFAICT I implemented everything that Fabianx and I discussed in #46. (With the exception of that duplicate() method, which Fabian already mentioned can easily be a follow-up, and I'd argue it must be, because it's strictly out of scope for this issue.)

With that, I'm signing off for the next 3 weeks — going on our honeymoon to Japan! :D

I'm still awaiting reviews. As far as I'm concerned, this is ready.

While awaiting that, here's additional test coverage to prove that this does not break the \Drupal\Core\EventSubscriber\ActiveLinkResponseFilter.

EDIT: why this extra test coverage? Well, upon re-reviewing this, I thought that was the only thing that could break. This test coverage proves it doesn't break.

I finally was able to finish reviewing this.

It is RTBC.

Random fail in Migrate_drupal_6.Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateTermNodeTest. Retesting.

Hm, happened again. Same random fail. I verified that D8.2 HEAD still passes tests, so it's not that HEAD is broken. I suspect a bad testbot? Retesting again. Let's see.

Once again that same random fail. Note that it includes PHPUnit_Framework_Exception: zend_mm_heap corrupted. I can't reproduce locally. Help :(

#65: zend_mm_heap corrupted is usually an opcache issue, where it runs out of memory or tries to free the memory twice.

It is usually a PHP 5.6 only bug usually and very hard to track down. (though in this case it is PHP 5.5)

Several sites I work with have experienced this in one fashion or another and usually increasing the memory size for opcache helps. And sometimes restarting php-fpm after deployments is the only help.

I am gonna ask Mixologic if we throw away the container for the apache or if we re-use it and just clean it. Though even then I am pretty sure we restart apache before the test.

Green on PHP 7! Failed on both 5.5 and 5.6. Retesting those, now that additional commits have been made to 8.2.x.

And now it's green on 5.5, but red on 5.6. Gaahhh!

Lets go back to RTBC and see what happens.

I queued a PHP 5.6 test for 8.3.

Looks like the same failure on PHP 5.6 on 8.2 and 8.3. Looks to me like it's maybe also the same failure as in #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x. I'm not clear if HEAD is failing the same way or not, of if there's something in this patch that's similar to what's triggering it in that one.

This can't be RTBC with a random fail being introduced, unfortunately. The only reason that the fail for Drupal\comment\Tests\CommentFieldsTest was not reverted is that it was introduced by a fix for a data loss critical. This is an experimental module.

However, I don't think this is actually the same fail as #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x? Are there usort() happening in this patch that I missed?

Oh, I do see https://www.drupal.org/pift-ci-job/443495 exactly above -- not the same bug elsewhere, but just the existing known fail. Is there a different one? Was there a different fail on 5.5? I don't see the other test results mentioned now. :( We should always document the exact fails, not only whether it is green or red.

In general, if a test failure does not have an existing critical for it, we should file one if it occurs in HEAD (and always check that list first). A test issue can be used to test whether it occurs in HEAD if needed.

What we could do is actually comment out CommentFieldTest in a testing patch based on this issue's patch, and queue multiple 5.6 runs for that test patch, to make sure the other 5.6 tests are all passing outside of the known bug in HEAD. However, that doesn't address the other fails that occurred before.

You can always look through http://drupal.org/node/3060/qa to see the past and present fails on HEAD.

#73: #61, #65 and #66 all talk about the same exact fail.

In general, if a test failure does not have an existing critical for it, we should file one if it occurs in HEAD (and always check that list first). A test issue can be used to test whether it occurs in HEAD if needed.

But it doesn't occur in HEAD. There's no sane/simple way to reproduce this, because it's something deep in the PHP engine. I have no idea at all how to reproduce this in a separate issue, other than posting this same patch. This fail was not occurring when Fabian RTBC'd this in #59.

@Wim Leers and it was not CommentFieldTest? CommentFieldTest fails in HEAD on 5.6 100% of the time. If that was the only thing that ever failed, we can mark this RTBC. If it is some other test though, or against some environment other than 5.6, that's when we should be concerned.

No, as I said, it was a migration test (specific one mentioned above). But that was with PHP 5.5. The fails are now isolated to 5.6, and are indeed for CommentFieldTest. But I'm pretty sure that the 5.6 fails *initially* were also in that same migration test.

(Also, shows clearly where DrupalCI falls short. We shouldn't have to manually write down exactly what failed.)

Do you want me to upload the same patch against 5.5 10 times?

I just read #2762549-54: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x, -55 and -56.

The thing is that PHP is apparently known to have memory corruption bugs that are only reproducible by certain combinations of code. Quoting DrupalCI maintainer Mixologic:

17:47:08 <WimLeers> isntall: Mixologic alexpott I'm seeing a consistent fail in MigrateTermNodeTest with zend_mm_heap corrupted. Reproduced 3 times on testbot. Cannot reproduce locally. The patch was green yesterday, it's red today. Help? :( https://www.drupal.org/node/2657684#comment-11565897
17:47:11 <Druplicon> https://www.drupal.org/node/2657684 => Ability to use BigPipe without a session: allow first sessionless (and hence anonymous) response to be delivered by BigPipe, then cached in Page Cache/Varnish/… for subsequent sessionless responses #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits => 65 comments, 3 IRC mentions
17:47:58 <Mixologic> WimLeers: when you say "cannot reproduce locally" do you mean on your env or using drupalci locally?
17:48:08 <WimLeers> Mixologic: my env
17:48:14 <WimLeers> I don't have DrupalCI locally
17:53:23 <Mixologic> WimLeers: Those look like php errors to me. I dont know what can be done about them. php, particularly on the olderish versions we test against, is broken. Something about those migrate tests are super effective at finding them.
17:54:23 <Mixologic> WimLeers: many times migrate tests fails are random, but something about those tests really needs to be re-evaluated.
17:56:14 <alexpott> Mixologic: fyi https://www.drupal.org/node/2791163#comment-11571013
17:56:16 <Druplicon> https://www.drupal.org/node/2791163 => Random automatic testing failures on SQLite with PHP 5.5 #2791163: Random automatic testing failures on SQLite with PHP 5.5 => 14 comments, 4 IRC mentions
17:56:37 → balsama joined  ↔ shawndearmond popped in  
18:06:18 <WimLeers> Mixologic: hmmm :(
18:06:34 <WimLeers> Mixologic: so you're basically saying "wait for some commits, it'll probably randomly go away again"
18:06:35 <WimLeers> ?
18:07:07 <Mixologic> WimLeers: kinda. php is segfaulting, because the version of php we test against and support has bugs.
18:08:04 <Mixologic> So random fails like the migrate test really mean we need to take a hard look at those migrate tests and figure out what it is about them that cause php to fail so often.
18:09:38 <Mixologic> WimLeers: the only other option is we switch our development testing to php7, and accept that while things are supported on older versions, people may run into bugs with older versions due to php, not due to drupal.
18:11:58 <Mixologic> This isnt a "something in the drupalci environment can be fixed" its somethings are broken with php 5.5.23, which isnt even the lowest version we support, but its not the newest version either.

Let me stress the most relevant bits:

WimLeers: so you're basically saying "wait for some commits, it'll probably randomly go away again"
Mixologic: kinda

Those fails disappeared now (since #68). SO YAY! That's "fixed". There's nothing special we need or can do for fixing those random fails that had memory corruption.

But now we have a different fail. And apparently (per #2762549: Drupal\field\Tests\Update\FieldUpdateTest, Drupal\views\Tests\Update\EntityViewsDataUpdateTest and Drupal\comment\Tests\CommentFieldsTest fail on 8.1.x-54]) it is known that that fails. Apparently that always fails, even on HEAD: https://www.drupal.org/node/3060/qa. I first didn't notice that that was a different fail.

Conclusion: all of xjm's concerns are addressed. This doesn't introduce new random fails. The random fails we had before were due to PHP bugs, that the DrupalCI team has seen many times before. The current fails I first assumed were the same, but they are apparently not; they're known fails. Because I believed that, the last few comments I made (#75 and #77) were basically just a distraction. Apologies!

Thanks @Wim Leers! Maybe we can get this in before RC after all, yay.

I queued 10 runs against 5.5 with the latest patch to make sure all is indeed well.

They all came back green :) Happy to see my words confirmed.

This patch makes changes to PageCache.php, so I'm not comfortable with it going into 8.1 at this point. I think 8.2 is fine, especially if we do it prior to 8.2.0. So, changing the issue's version accordingly.

Given that 8.2 RC is almost upon us, I wanted to first review the changes made to code outside of the Big Pipe experimental module. Here it is:

1. +++ b/core/lib/Drupal/Core/Render/CacheableStreamedResponseEventDispatcherTrait.php
   @@ -0,0 +1,105 @@
   + * @internal
   + * @todo Make public once code/modules other than BigPipe start using this.
   + */
   +trait CacheableStreamedResponseEventDispatcherTrait {
   

   Do we need this trait to be in Drupal/Core/Render during 8.2? I think my preference would be to put it into the big_pipe namespace for 8.2, then have an 8.3 only issue to move it into Drupal/Core/Render. My reason for that will make more sense at the end of this comment, but I'm open to arguments for why 8.2 contrib would benefit enough from this trait to make it worth rushing in.

2. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -81,6 +83,19 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
   +  public function terminate(Request $request, Response $response) {
   

   In addition to what this method is doing in this patch, it MUST also delegate, per https://github.com/stackphp/builder/issues/14.

3. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -206,6 +221,43 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +    // 'terminate' phase/event. Already set the 'X-Drupal-Cache' header, so that
   +    // header is present as expected.
   

   The last sentence is confusing. What does "already" mean in this context? Where else can that header be set, since the terminate event can't set headers, given that it fires after the response is already sent? And given that, I think the sentence can be removed entirely.

4. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -206,6 +221,43 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +    // Allow policy rules to further restrict which responses to cache.
   +    if ($this->responsePolicy->check($response, $request) === ResponsePolicyInterface::DENY) {
   +      return $response;
   +    }
   

   This should move into the storeResponse() method, since policies should also be checked for CacheableStreamedResponseInterface responses.

5. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -275,8 +314,6 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
        // Mark response as a cache miss.
        $response->headers->set('X-Drupal-Cache', 'MISS');
   

   Meanwhile, this should move out of the storeResponse() method and into the fetch() method, since per point #2 above, we shouldn't be messing with headers during the terminate() stage.

Since I haven't yet sufficiently reviewed the changes to Big Pipe itself, I'd like to request that a child issue be made with all of the changes outside of Big Pipe, with the above feedback addressed. That I would feel comfortable committing into 8.2 prior to RC (especially if the trait isn't included per point #1). Then, given that Big Pipe is still experimental, I think its changes could be committed during the RC phase, or even into 8.2.1, unless a release manager disagrees with that.

+1 for 8.2-only.

Had a call with @effulgentsia about #81, and how he'd like to see it addressed.

1. Discussed & fixed: indeed moved it to the BigPipe module.
2. Discussed & fixed, and since there's no existing unit test coverage for PageCache, we felt it was best to have a follow-up to add unit tests for all Stack middlewares: #2795199: Unit tests for all Stack middlewares.
3. Discussed & fixed.
4. Discussed & fixed. Given that this shows there's a bug, I felt explicit test coverage for this was an absolute requirement, so did that.
5. Discussed & fixed.

Furthermore:

1. @effulgentsia asked to minimize changes to PageCache, by being more careful with moving of code.
2. And I noticed a missing @returns docblock and incorrectly ordered use statements.

In this reroll, only adding the test coverage to address #81. In the next reroll, you'll find the fix.

And here's the fix. (Note that the moving of code to minimize changes to PageCache actually make this interdiff look more complex than it is.)

I'd like to request that a child issue be made with all of the changes outside of Big Pipe, with the above feedback addressed. That I would feel comfortable committing into 8.2 prior to RC (especially if the trait isn’t included per point #1).

Done: #2795209: Allow streamed responses to be cached by Page Cache.

To the delegation of Middlewares:

Would that not break our code that already traverses all terminable middlewares one by one and calls terminate() on them?

I feel that is a needed follow-up discussion, but has no place in this issue?

Edit:

Discussed in IRC:

The change in https://github.com/stackphp/builder/commit/17887ecd4112b04d8d9c5327bc05b... StackPHP was technically a BC break, as it broken one assumption (its own) and replaced with what projects did.

We had added Stack before that commit, which is why I still had the unconditional foreach in mind.

IMHO the change makes no sense at all (as the terminable part is not decorated anyway due to it being a conditional interface), but as it was done, we have no other chance, than to comply ...

.

Back to RTBC.

Good catch @effulgentsia. I did not realize this patch included changes outside of BigPipe; that is unfortunate. Changes like adding interfaces and functionality to stable core should have only been targeted against the minor version, which would have meant 8.3.x as of beta1 a month ago. In Outside In, we asked that such changes be moved to separate issues scoped to the problem they were trying to solve, and that they be done before beta. In general issues for an experimental module should not include any changes to stable code at all.

I guess that is what #2795209: Allow streamed responses to be cached by Page Cache is? But that issue is not beta-eligible, much less rc-eligible.

Also, this issue is not RTBC then.

Actually postponed on the blocker, I guess.

:( :( :(

It's contained that code since #13 (almost 3 months ago), i.e. since the first version of this patch. Since the changes to page_cache make zero sense without the rest of this patch, I didn't think of moving it into a separate issue. It's one logical/semantical unit/commit.

So, alas, this will be a big BigPipe feature for 8.3 then.

But I agree with core committers that regardless of risk, committing changes to page_cache would be violating the rules and semver principles that allow us to have those nice frequent releases.

I opened #2795391: Move most of PageCache::fetch() into a storeResponse() helper that can be independently invoked from a subclass and hope that that is still reasonable to get into an 8.2 patch release, if not during RC. If that's allowed, then I think this issue can still be made to work for 8.2, by having the Big Pipe module swap out the http_middleware.page_cache service with a subclass of PageCache. Therefore, moving this back to 8.2.x for now.

I discussed #93 with @catch and he'd prefer BigPipe to land into 8.3 cleanly first (meaning not needing to subclass PageCache). While he's theoretically open to an 8.2 backport after that, he'd also be fine without such a backport. Therefore, moving this back to 8.3.

I was testing RefreshLess recently and noted that it wasn't working for anonymous users, after finding this issue I realise this was probably just sessionless users. So I was just wondering if this issue would help with that situation.

1. Are these changes "compatible" with the idea of allowing RefreshLess to work with anonymous/sessionless users?
2. If yes, will this patch just magically allow it to work?
3. Or will it just provide the groundwork?
4. Alternatively are these completely unrelated issues and I'm looking for answers in the wrong place?

#95 It might be related, but its hard to say. Best would be to open a support request for refreshless with your use case.

Postponed now on #2795391: Move most of PageCache::fetch() into a storeResponse() helper that can be independently invoked from a subclass

Why this still postponed?

No longer postponed - indeed.

I've resumed working on this earlier today. Expect an update soon.

So, I started out with rerolling this patch. Then read #2795391: Move most of PageCache::fetch() into a storeResponse() helper that can be independently invoked from a subclass and looked at how that impacted this patch. Which led me to read @znerol's comment at #2795209-28: Allow streamed responses to be cached by Page Cache. In there, he formulates his concerns/criticism on the impact of modifying PageCache to become aware of the new \Drupal\Core\Render\CacheableStreamedResponseInterface that this issue introduces. Let me quote it in full here:

@znerol at #2795209-28:

I did read through the following material today:

1. $ (cd src/symfony && git grep -il streamed | grep -vi test)
   src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php
   src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
   src/Symfony/Component/HttpFoundation/CHANGELOG.md
   src/Symfony/Component/HttpFoundation/Response.php
   src/Symfony/Component/HttpFoundation/StreamedResponse.php
   src/Symfony/Component/HttpKernel/CHANGELOG.md
   src/Symfony/Component/HttpKernel/Client.php
   src/Symfony/Component/HttpKernel/EventListener/SaveSessionListener.php
   src/Symfony/Component/HttpKernel/EventListener/StreamedResponseListener.php
   src/Symfony/Component/HttpKernel/Fragment/FragmentHandler.php
   src/Symfony/Component/Templating/DelegatingEngine.php
   src/Symfony/Component/Templating/StreamingEngineInterface.php
   

2. $ (cd src/drupal && git grep -il streamed | grep -vi test)
   core/modules/big_pipe/js/big_pipe.js
   core/modules/big_pipe/src/Render/BigPipeResponse.php
   core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
   core/modules/file/file.module
   core/modules/page_cache/src/StackMiddleware/PageCache.php
   

3. $ (cd src/drupal && find core/modules/big_pipe -name '*.php' | grep -vi test)
   core/modules/big_pipe/src/Controller/BigPipeController.php
   core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
   core/modules/big_pipe/src/EventSubscriber/NoBigPipeRouteAlterSubscriber.php
   core/modules/big_pipe/src/Render/BigPipe.php
   core/modules/big_pipe/src/Render/BigPipeInterface.php
   core/modules/big_pipe/src/Render/BigPipeMarkup.php
   core/modules/big_pipe/src/Render/BigPipeResponse.php
   core/modules/big_pipe/src/Render/BigPipeResponseAttachmentsProcessor.php
   core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
   

4. #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits
5. #2577631: Allow HtmlResponse to use a flexible emitter
6. https://en.wikipedia.org/wiki/Chunked_transfer_encoding
7. https://info.varnish-software.com/blog/http-streaming-varnish
8. https://www.varnish-cache.org/docs/4.1/reference/vcl.html?highlight=do_s...

My observations/thoughts so far:

1. It looks like Symfony treats streamed responses slightly different than Drupal (might be a bug over here). Especially send() is invoked from within a response listener and thus the callback is executed in the context of a normal request/response cycle. In a Symfony application, the streamed response is already built/sent to the browser when $kernel->handle() returns. Hence a middleware wrapping the kernel could theoretically access that data. However, subclassing StreamedResponse is deprecated in Symfony 3 since that class will be final in Symfony 4.
2. FrameworkBundle/Controller/Controller.php shows an example on how the callback-approach is supposed to work. I think BigPipe could do something similar.
3. BigPipe is marked as experimental and I think nobody will deny that the code still has some rough edges. It looks like the emitter idea could help cleaning it up. How much cleaning up is required until we can drop the experimental label from BigPipe? If cleaning up results in less changes outside of BigPipe, then let's do that first.
4. I do not quite understand how the problem that cacheability metadata cannot be communicated properly to external proxies stated in #2657684-25: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits is resolved in the current patch. I understand that this is only tangibly related to the problem here. Maybe this is solvable with trailers as described in RFC 7230 4.1.2. Not sure whether varnish parses trailers though.
5. Generally I think that we should first try to store big pipe responses in the dynamic page cache. IMHO this is a natural fit (and that's what is already implemented for big pipe responses with a session AFAIK). That approach will work as well for the advanced use-case described in the issue summary.

I just posted a response to that at #2795209-31: Allow streamed responses to be cached by Page Cache, also quoting verbatim:

#28

4. Thanks for pointing us to trailers in the RFC! I've always wondered about that, but had never heard of it before. https://www.varnish-cache.org/trac/wiki/HTTPFeatures suggests Varnish supports it. But https://github.com/whatwg/fetch/issues/34 shows that it's a small miracle that Varnish supports it. Many CDNs support it according to the feedback in that GitHub issue. See also https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/TE for more details about how it's supposed to be used.
   However, Symfony's HTTP Foundation has precisely zero support for trailers. :( And, nobody has ever even mentioned let alone requested it: https://github.com/symfony/symfony/search?utf8=%E2%9C%93&q=trailers.

   A) Trailers + CacheableStreamedResponseInterface

   I think @znerol is entirely right that this is the correct solution from a HTTP spec perspective. If we'd use this, then we wouldn't need CacheableStreamedResponseInterface like #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits and this issue propose!

   So I think it's clear that we should not modify PageCache: anything it does must rely on the contents of the HTTP request & response only, otherwise it ends up doing things that no other reverse proxy can do. The whole point of PageCache is that it's just a poor man's Varnish — i.e. that it doesn't have any special knowledge.
   

   B) Streamed responses + cache tags

   @znerol is also entirely right in his implicit concern/claim that it's silly that we'd still support Drupal <> PageCache <> reverse proxy but not Drupal <> reverse proxy. As #2657684-28: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits explained, the former would still work because request 1 streams it and stores it in Page Cache, request 2 then has a page cache hit, and request 3 is a reverse proxy hit. It is thanks to Page Cache that we can get the streamed response to be cached by the reverse proxy with all its cache tags. In the latter case, it's impossible to get all cache tags for the streamed response to be known to the reverse proxy (unless it supported trailers).

   This also means I disagree with @Fabianx' comment at #22: So Varnish already does what we want to do in PageCache. Stream the response and then store it in the cache. — this is misleading, because it's omitting the fact that Varnish will not have cache tags for the streamed placeholder fragments, but Page Cache will. Which means Varnish will not be able to invalidate when possible, but Page Cache will.

   Once #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits lands, you won't be able to use BigPipe + CDN without also using Page Cache.
   

   Conclusion: our options for proceeding

   For A), I'd vote to remove that interface altogether, and just let the BigPipe module write a response to the Page Cache directly, just like @znerol asked in #19, and with which I disagreed in #20. We shouldn't embed logic that's not using data in a HTTP request/response in Page Cache, because that makes it no longer be an actual HTTP middleware.

   For B), there's no clear choice.

   Our options for proceeding:

   1. Proceed with the old plan, and just document that you MUST use Page Cache when using a reverse proxy that uses tag-based invalidation. This seems unwise.
   2. only let BigPipe stream responses for anonymous users IF $settings['reverse_proxy'] !== TRUE OR $settings['reverse_proxy'] === TRUE && module_exists('page_cache')… i.e. option 1, but enforced via code instead of documentation. This means it's still fairly difficult to support, because people forget to set that in settings.php and it introduces difficult-to-debug failure modes for BigPipe when used with CDNs
   3. only let BigPipe stream responses for anonymous users IF the response would not be cacheable by Page Cache anyway Impossible, because in many cases, it depends on a response policy to determine whether something can be stored in Page Cache, i.e. we need the response to be generated, i.e. it's too late to choose to use BigPipe to deliver the HTML, because all the work has already been done
   4. move the entire "BigPipe streaming + PageCache" logic to a contrib module: big_pipe_sessionless or something like that… but this would require significant extending/overriding of "core BigPipe". But at least supporting "core BigPipe", which provides 95% of the benefits, could be absolutely rock-solid (as it has been until now — effectively no bug reports).

5. Storing in Dynamic Page Cache is what we already do, both for anonymous and authenticated responses. The point of this issue (and #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits) is to make it possible to have anonymous responses be served with a single flush upon a cache miss, but to stream them instead. Subsequent requests as the anonymous user would then again benefit from the Page Cache. What you're suggesting is that we never cache those responses in Page Cache, which is correct, but leads to worse performance overall: the first response is faster, but all responses after that are slower. So that would then clearly miss the point :)

In other words: thank you @znerol for raising those concerns! You've convinced me that the current state/direction of this patch are a bad idea.

So, of the 4 options I identified for proceeding:

1. Option 1 seems a no-go to me.
2. Option 2 is viable, but still means not using trailers (cfr. A)), which is what we should be using. And it'll be painful to support.
3. Option 3 was marked impossible.
4. Option 4 seems the best decision when you factor in maintainability (both in terms of possible failure modes and in terms of code size: 25 files changed, 764 insertions(+), 81 deletions(-)). It would also mean we can close this issue, and can move immediately to #2797169: Mark BigPipe as stable/non-experimental.

With that, I'm assigning this to @Fabianx and @catch for feedback.

I'd need to get up to speed with this patch again and with the comments referenced in #101 to post a more thorough comment, but for now...

move the entire "BigPipe streaming + PageCache" logic to a contrib module: big_pipe_sessionless or something like that… but this would require significant extending/overriding of "core BigPipe"....[this] seems the best decision when you factor in maintainability...It would also mean we can close this issue, and can move immediately to #2797169: [PP-1] Mark BigPipe as stable/non-experimental.

I think big_pipe_sessionless being either in contrib or in a core alpha-stability submodule is fine. But so long as it requires significant overriding of what's in big_pipe module, I don't think we should promote big_pipe to stable. I'd rather us figure out what we need to do to core big_pipe module in order to make a big_pipe_sessionless module possible without "significant" overrides. "Significant" is of course a judgment call and a sliding scale.

it's silly that we'd still support Drupal <> PageCache <> reverse proxy but not Drupal <> reverse proxy

Yeah, this definitely feels weird. Long term, it would be great if big_pipe_sessionless could figure out a way to not have a hard dependency on page_cache, but especially while it's an experimental module, I think it's fine for it to have that dependency.

concerns/criticism on the impact of modifying PageCache to become aware of the new \Drupal\Core\Render\CacheableStreamedResponseInterface that this issue introduces.

If an architecture that completely decouples from middleware is too hard, could we at least make big_pipe_sessionless extend/decorate Drupal\page_cache\StackMiddleware\PageCache rather than needing to apply the questionable changes directly to page_cache module?

But so long as it requires significant overriding of what's in big_pipe module, I don't think we should promote big_pipe to stable

I disagree with this rationale. To make advanced contrib modules that override/extend parts of core easier to implement, we refactor internals of affected core modules as necessary. I don't think it needs to be different here.

Long term, it would be great if big_pipe_sessionless could figure out a way to not have a hard dependency on page_cache, but especially while it's an experimental module, I think it's fine for it to have that dependency.

But that's physically impossible until Symfony and Page Cache gain trailer support. Once they both gain trailer support, that changes things. Then sessionless BigPipe can work with reverse proxies that support trailers, without needing page_cache.
In other words: in that case, page_cache is just like any other reverse proxy again: any reverse proxy that supports reading the cache tags trailer (header) will work fine.

could we at least make big_pipe_sessionless extend/decorate Drupal\page_cache\StackMiddleware\PageCache rather than needing to apply the questionable changes directly to page_cache module?

Yes, of course. That's what I said in my response to #28.4.A): So I think it's clear that we should not modify PageCache: anything it does must rely on the contents of the HTTP request & response only, otherwise it ends up doing things that no other reverse proxy can do. The whole point of PageCache is that it's just a poor man's Varnish — i.e. that it doesn't have any special knowledge.

While awaiting the response of @effulgentsia, @Fabianx and @catch, I'll already get started on a big_pipe_sessionless module, so I can properly observe where pain points are, and where we can refactor BigPipe internals to make such a module more feasible.

This addresses #102. The attached patch includes all changes I'd propose to make as part of this issue. They make it possible for contrib modules like "BigPipe Sessionless" to work. All code changes in fact improve legibility of the existing code!

This interdiff (interdiff-everything.txt):

1. reverts all core/modules/page_cache changes
2. reverts all sessionless-functionality changes in core/modules/big_pipe
3. moves all sessionless-functionality chagnes to modules/big_pipe_sessionless

The end result is:

1. big_pipe_no_session_pave_path_for_contrib-2657684-104.patch, which contains all of the above, minus the big_pipe_sessionless module, see interdiff.txt for just this portion
2. big_pipe_sessionless.module-2657684-104-do-not-test.patch, which contains the new module, 99% finished, and fully verified to be operational

Back to you, @effulgentsia. I think/hope this convinces you.

Awesome! This is just a review of the big_pipe changes, not big_pipe_sessionless.

1. +++ b/core/modules/big_pipe/src/Render/BigPipeInterface.php
   @@ -134,11 +134,9 @@
   -  public function sendContent($content, array $attachments);
   +  public function sendContent(BigPipeResponse $response);
   

   This is exactly the kind of API change I'd much rather make before marking big_pipe stable than after. However, big_pipe is already documented as beta stability, so I think this might be worth doing in its own issue for greater visibility both during discussion and in the commit log.

2. +++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
   @@ -25,6 +25,68 @@ class BigPipeResponse extends HtmlResponse {
   +  public function __construct(HtmlResponse $response) {
   

   This seems like an API change to me as well, so same point as above for sendContent(). https://www.drupal.org/core/d8-bc-policy excludes constructors from the API only for service objects, plugins, and controllers, which this isn't one of.

3. +++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
   @@ -91,39 +91,25 @@ public function onRespond(FilterResponseEvent $event) {
   -    $big_pipe_response->setPrivate();
   

   I'm not clear on whether this is an API change, a behavior change, or just a no-op clean-up. What's the reason for it?

4. +++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
   @@ -38,7 +100,15 @@ public function setBigPipeService(BigPipeInterface $big_pipe) {
   +    // All BigPipe placeholders are processed, so update this response's attachments.
   +    if (isset($this->attachments['big_pipe_placeholders'])) {
   +      unset($this->attachments['big_pipe_placeholders']);
   +    }
   +    if (isset($this->attachments['big_pipe_nojs_placeholders'])) {
   +      unset($this->attachments['big_pipe_nojs_placeholders']);
   +    }
   

   Looks like this is being added, rather than moved from elsewhere. If HEAD is working without this being done, what about this patch makes this now necessary?

Regarding API changes in BigPipe in general: I created #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal.

1. This issue has 37 followers. It's the open BigPipe issue with the most followers by far. We'll never get more visibility than here. Also, we're making this change precisely to support the original goal of this issue in contrib. So I'd rather make this change here.
2. See #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal.
3. Good catch! This is an oversight. Drupal already is setting Cache-Control: private by default, which is why the test passed. Still, we should add back that line just to
4. This is merely for consistency with #2597359: Require responses with attachments to contain the final attachment values. i.e. to ensure that the final response object's attachments contains only attachments that have not yet been processed. This is matching the behavior of \Drupal\Core\Render\HtmlResponseAttachmentsProcessor and \Drupal\Core\Ajax\AjaxResponseAttachmentsProcessor. I'd be happy to move this to a separate issue though, it's minor clean-up.

So, if we look at this patch, we can summarize the code changes as follows:

• HtmlResponseBigPipeSubscriber: simplify, by moving logic into BigPipeResponse. The moved logic 100% applies to BigPipeResponse. This therefore makes BigPipeResponse more self-contained.
• BigPipe: rather than print $chunk; flush();, call the new sendChunk($chunk) helper method. In big_pipe_sessionless, we can then subclass this and also track all sent chunks. Also introduce perform(Pre|Post)SendTasks(), to perform tasks before and after streaming (sending) content. In BigPipe, we use this to start/stop the session (for placeholders whose content depends on the session). In big_pipe_sessionless, they are overridden to do nothing.
• BigPipeInterface: change sendContent(string $content, array $attachments) to sendContent(BigPipeResponse $response). This allows big_pipe_sessionless to access other data. Plus, it just makes sense: pass the BigPipe service (which is really the heart of the BigPipe module) the BigPipeResponse value object, rather than just seemingly arbitrary subsets of it.
• BigPipeResponse: change the constructor to receive the original HTML response (necessary to allow the logic to be moved into this class, out of HtmlResponseBigPipeSubscriber), plus the ability to access the original response (necessary for big_pipe_sessionless, so that it can construct a correct "non-streamed" HTML response of the streamed content for caching in Page Cache), plus the minor clean-up mentioned in #106.4.
• BigPipeStrategy: move a small bit of logic into a helper function, so that big_pipe_sessionless does not have to duplicate it.

All of this is in fact making the BigPipe module cleaner!

Finally, looking at the test changes, we:

1. add test coverage to verify that BigPipe responses do NOT contain the cache tags bubbled up by placeholders whose contents are streamed
2. the changes in BigPipeTest::getTestCases() and BigPipeTestController::test() should arguably be removed, but they'd require needless duplication in big_pipe_sessionless

Hopefully this simplifies the review process.

This removes the unnecessary changes in BigPipeTest::getTestCases() that I mentioned in #107.tests.1.

I now finished the big_pipe_sessionless module, by moving #108 over to 8.3.x, where PageCache::storeResponse() is available.

In interdiff-everything.txt, you can see:

1. one additional small bit of refactoring in BigPipe: extracted a new protected helper method
2. removed the unnecessary changes in BigPipeTestController::test() that I mentioned in #107.tests.2
3. big_pipe_sessionless now depends on Drupal 8.3.0
4. big_pipe_sessionless now alters the Page Cache middleware class, to expose a public function that allows it to store a response
5. clean-up of big_pipe_sessionless

In interdiff.txt, you can see all changes that affect core's big_pipe module:

1. See point 1 above.

big_pipe_no_session_pave_path_for_contrib-2657684-109.patch is still the patch to review. interdiff.txt is still the interdiff to review.

big_pipe-AND-big_pipe_sessionless-2657684-109.patch and interdiff.txt are posted only to prove that it's possible to implement this in contrib.

For core, this patch now means 8 files changed, 204 insertions(+), 76 deletions(-) instead of 8 files changed, 209 insertions(+), 74 deletions(-) (#108) or even 25 files changed, 764 insertions(+), 81 deletions(-) in the previous direction (#83).

The patch in #109 that included big_pipe_sessionless failed in Drupal\system\Tests\Module\InstallUninstallTest. Just because that test is also installing contrib modules. Its failure is irrelevant, because what matters is that its test coverage passed, which it did:

…
11:03:55 Drupal\Tests\big_pipe_sessionless\Unit\Render\Placeholder\Bi  10 passes                                      
…
11:05:02 Drupal\Tests\big_pipe_sessionless\Functional\BigPipeSessionl 333 passes                                      
…

So, #109 proved that this module can work in contrib.

In this new scope+direction:

1. The title needed an update. Did that.
2. The issue summary needed an update. Did that.
3. This no longer needs any documentation changes.
4. The draft CR should be deleted. (I'll do that once we have consensus.)

Do we have consensus that this functionality does not belong in core, because of the rationale I gave in #101?

#2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal was committed. This needed a reroll, there was a small conflict in one docblock.

Just quickly:

I think it makes sense to put the no-session stuff in contrib.

Using trailers would be fantastic, really like that idea if we can sort out support.

Patch itself looks like good cleanup but haven't done a proper review yet.

Thanks, @catch, that's very encouraging!

Assigning to @Fabianx for feedback too.

i reviewed both the big_pipe_sessionless and the patch (in the whole history) and it all looks perfect.

All the helper functions make the code more readable, easier to understand and manageable.

My main concern that Varnish could be caching the big pipe response is also unfounded as only the HtmlResponse is marked cacheable - due to how its encapsulated.

A little bit of a pity that we can't ship with this by default due to support for reverse-proxies / CDN and not being able to rely on reverse_proxy setting, but the good enws is that as a user that does not use Varnish you can just use the contrib module then and its also really nice that with that you can just choose whatever big_pipe you want.

This also paves the way for further authcache_big_pipe implementations - where the same trick can be used and it would just need to override the subscriber, yet again ;).

So everything looks fantastic and as I found nothing in my code review either:

This is even RTBC!

Really really great work!

Random testbot fail? When I came to this issue, #114 was green again.

Just a few nits... this looks good.

1.    *   This method should only be invoked by
      *   \Drupal\big_pipe\Render\BigPipeResponse, which is itself an internal
      *   class. Furthermore, the signature of this method will change in
      *   https://www.drupal.org/node/2657684.
   

   That comment needs updating.

2. +++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
   @@ -28,6 +28,73 @@ class BigPipeResponse extends HtmlResponse {
   +   * @return \Drupal\Core\Render\HtmlResponse
   +   */
   

   Missing text description.

3. +++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
   @@ -41,7 +108,15 @@ public function setBigPipeService(BigPipeInterface $big_pipe) {
   +    // All BigPipe placeholders are processed, so update this response's attachments.
   

   Needs wrapping.

4. +++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
   @@ -41,7 +108,15 @@ public function setBigPipeService(BigPipeInterface $big_pipe) {
   +    if (isset($this->attachments['big_pipe_placeholders'])) {
   ...
   +    if (isset($this->attachments['big_pipe_nojs_placeholders'])) {
   

   These ifs are actually unnecessary.

5. +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
   @@ -169,6 +169,21 @@ protected function doProcessPlaceholders(array $placeholders) {
   +   * @param $placeholder
   

   Is this a string - I guess so. Missing the type.

1. Excellent nitpick!
2. Added.
3. Wrapped at 80 cols.
4. These ifs are definitely necessary. Grep the codebase for isset($attachments['big_pipe_placeholders'] and you'll see more occurrences. The reason: some requests/responses use only no-JS placeholders, others use only JS placeholders.
5. Added typehint. And yes, definitely a string, there's even an assert() for it to guarantee that. :)

100% of the changes are comments only, so back to RTBC, and must still be green.

Re #120.4 https://3v4l.org/PVbMt unset doesn't care if the thing exists on not. Imo it just makes for more readable code ... less to read / care about.

Aha! I didn't know that PHP edge case. I think the current code is clearer, but I don't feel strongly about it. Done.

Deleted the CR, since it's no longer relevant: this is now solely refactoring of internals, which does not need or merit a change record.

Committed 69c2347 and pushed to 8.3.x. Thanks!

Woot! This unblocks the trivial #2835758: Remove BigPipeInterface and move all of its docs to the implementation, which is the last blocker for #2797169: Mark BigPipe as stable/non-experimental :) :) :)

Moving it to a separate module made it possible to clearly express the dependency on the Page Cache module for this functionality. So it should not be called "BigPipe Sessionless", but:

• BigPipe Page Cache Populator
• BigPipe Page Cache Primer
• BigPipe Page Cache Accelerator
• BigPipe Streamed Page Cache Misses
• … something like that

I want to move the code in #109 to a contrib module, but of course we need to settle on a name first. Feedback is much appreciated, especially from BigPipe co-maintainer Fabianx!

IMO, that big_pipe_sessionless depends on page_cache and works by priming it directly (breaking the encapsulation of page_cache writes happening solely in middleware) is all implementation details that should be allowed to change without requiring a change to the module name. E.g., if HTTP response trailers become better supported, then those details might change in a way that "BigPipe Page Cache Primer" would no longer be an accurate name.

So, I think big_pipe_sessionless is an ok name, since it reflects the purpose of the module (to provide BigPipe capability to sessionless requests) regardless of its implementation. Maybe the human-friendly name in the .info.yml should be "Sessionless BigPipe"? There might be ideas for better names, but none are coming to my mind at the moment.

My only question to #134 though is, would big_pipe_sessionless ever evolve into handling requests with sessions? E.g., in configurations where a reverse proxy can cache responses for requests with sessions? My hunch is no, and that even with such reverse proxies, we would still have big_pipe handling requests with sessions, and big_pipe_sessionless handling requests without sessions, but I'm not 100% confident on whether that's correct.

The code in #109 is now published in a contrib module at https://www.drupal.org/project/big_pipe_sessionless. I created a commit history to match the comment history here.

8.x-1.0 release imminent :)