Allow streamed responses to be cached by Page Cache

RTBC - Looks great to me!

I'm really sorry that I did not spot these changes in the BigPipe patch sooner -- these changes would have had a beta deadline for 8.2.x. I don't think marking the interface internal really gets around that.

I definitely agree with splitting them into their own issue, though.

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

   Is the reason for marking this interface internal that:

   1. We think we will need to change it,
   2. We don't want people to try using it, or
   3. We don't want to break public API by adding it?

   If it's #1, then I think marking it internal is okay. If it's #2 I don't see how the @todo is going to happen since it's internal code. :) If it's #3 only we should definitely just consider adding it as a public interface to 8.3.x, because marking an interface internal doesn't really get around the fact that this change would have needed to go in before beta.

2. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -206,6 +226,39 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   +    // @see ::terminate()
   

   IIRC API.d.o does not pick up this syntax, but OTOH I think it also doesn't parse @see in inline comments anyway.

3. +++ b/core/modules/page_cache/src/StackMiddleware/PageCache.php
   @@ -228,7 +281,7 @@ protected function fetch(Request $request, $type = self::MASTER_REQUEST, $catch
   -      return $response;
   +      return FALSE;
   

   So reading this line out of context makes it look like quite a BC break. I'm assuming the actual code flow still results in the $response being returned in most circumstances? Because this is actually moving code that used to be in the main method into a helper method? Is that correct?

I'm moving the issue to 8.3.x now, which catch also suggested was the right thing to do. (I don't believe he's had the chance to look at the actual code here yet.)

Re #5.1: This interface also has a @todo pointing to #2577631: Allow HtmlResponse to use a flexible emitter. I think whether it should be marked @internal depends on whether we want to leave the option to change anything about the definition of getCacheableStreamedResponse() as part of that issue or any other possible refactoring. For example, are we confident about wanting to commit to the @return type that's documented on this method and also the docs that say that callers can rely on KernelEvents::RESPONSE having been dispatched? If we are confident about those requirements, then I think we can remove the @internal. Because if all we plan to do in the future is additions to this interface, that's still allowed per https://www.drupal.org/core/d8-bc-policy#interfaces so long as we don't add the @api tag to it. If we want to leave room to change that, then let's keep it @internal and document that we reserve the possibility to remove or change that method definition as part of #2577631: Allow HtmlResponse to use a flexible emitter.

Re #5.2: So that just needs to change to static::terminate(), I think.

Re #5.3: Right, there's no change to what is returned by fetch(), just the moving of code into a helper method with a different @return signature, so that it can be called from terminate() as well as fetch().

Setting to NW primarily for #5.1, which needs something done per #7.

While we're at it, might as well fix #5.2 per #7. And also:

+++ b/core/lib/Drupal/Core/Render/CacheableStreamedResponseInterface.php
@@ -0,0 +1,27 @@
+   * @see \Drupal\Core\Render\CacheableStreamedResponseEventDispatcherTrait

The trait isn't in this patch, so this line needs to be removed.

Adding review credit to @Fabianx and @xjm.

+++ b/core/modules/page_cache/tests/modules/page_cache_test/src/PageCacheTestCacheableStreamedResponse.php
@@ -0,0 +1,25 @@
+class PageCacheTestCacheableStreamedResponse extends Response implements CacheableStreamedResponseInterface, CacheableResponseInterface {
+
+  use CacheableResponseTrait;
+
+  protected $streamedResponse;
+
+  public function __construct($streamed_response) {
+    $this->streamedResponse = $streamed_response;
+    parent::__construct('', 200, []);
+  }
+
+  public function getCacheableStreamedResponse() {
+    return $this->streamedResponse;
+  }
+}

I was curious if Coder was going to complain about the lack of class or function docs here, but it didn't. All it complained about was a missing newline between the last 2 braces, so here's a fix for that.

One more trivial fix for a Coder complaint.

I'm moving the issue to 8.3.x now, which catch also suggested was the right thing to do.

I'm happy with #12 and am comfortable committing it to 8.3 once it's re-RTBC'd (interdiffs since #5 reviewed and approved by someone).

I'm sad for this not to make it into 8.2. https://www.drupal.org/core/d8-allowed-changes#beta includes a bullet for "minor API additions or internal API changes to fix bugs (or for other prioritized changes), if the impact outweighs the disruption", and IMO:

• This qualifies as a prioritized change due to it unblocking #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 for commit to an 8.2 patch release or during 8.2 RC, which if that's allowed for changes to experimental code only, is a nice way to improve performance for the next 6 months on production sites using BigPipe and encountering frequent page cache misses.
• This patch's new @internal interface qualifies as a minor API addition, if it's even that, and since no one needs to use that interface, has 0 disruption.
• This patch's change to PageCache to implement TerminableInterface is a backwards-compatible behavior addition that I would expect to have 0 to minimal disruption (while it's theoretically possible for a contrib/custom module to for some reason be relying on it not implementing that interface, that seems like a very brittle assumption, and one we shouldn't be expected to coddle).

However, even if all that is true, we're pretty much at the tail end of the beta period, and RC is imminent, so if the release managers don't feel comfortable with this going into 8.2 so last minute, then I respect that call. In which case, I opened #2795391: Move most of PageCache::fetch() into a storeResponse() helper that can be independently invoked from a subclass in the hopes that at least that much can make it into 8.2 (either before or during RC, or into a patch release), which would allow for BigPipe to implement its own subclass of PageCache and add the terminate() method to that. In which case, CacheableStreamedResponseInterface could also be moved to the BigPipe namespace for 8.2.

which would allow for BigPipe to implement its own subclass of PageCache

Note, however, that that would not be compatible with a contrib/custom module that swaps out http_middleware.page_cache for a custom implementation. Either BigPipe or that other module would "win" and likely not accommodate whatever the other one needs done. But that's an edge case that we could leave to that contrib/custom module and the site that uses it to solve, since BigPipe is after all still experimental. In that regard though, committing #12 to 8.2 is cleaner, but that's not necessarily sufficient reason to do it.

I'm sad for this not to make it into 8.2

Don't be sad. This makes Drupal 8.3.x better, and now is the very best time to start making Drupal 8.3.x awesome. Also, BigBipe can still be beta in 8.2.0, RC soon, and stable for 8.3.0. It will be the first experimental module in beta and the first one in stable!

This qualifies as a prioritized change due to it unblocking #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 for commit to an 8.2 patch release or during 8.2 RC, which if that's allowed for changes to experimental code only, is a nice way to improve performance for the next 6 months on production sites using BigPipe and encountering frequent page cache misses.

While the BigPipe feature is a great one, that doesn't make that issue a prioritized change. Outside In and Content Moderation were prioritized changes. (I think we might remove those four words from the policy doc for the next beta, since they seem to confuse people about the policy more than they help, and they necessarily meant something different when beta lasted a year.)

we're pretty much at the tail end of the beta period, and RC is imminent, so if the release managers don't feel comfortable with this going into 8.2 so last minute, then I respect that call.

The last-minute-ness definitely doesn't help (nor does the fact that today is a US holiday), but I would have made the same decision even a few weeks ago.

has 0 disruption

Let's avoid saying that in general. A spelling fix in a docblock might be close to 0 disruption within some rounding error. Things that are near 0 disruption also tend to be near 0 impact. :) Also keep in mind improvements to page caching and low-level subsystems inherently carry more risk, even BC ones. Not saying the patch is particularly high risk either, and it's BC with internal API additions, but in any case those are changes we target for minor releases and that therefore have a beta deadline.

Those aren't the main reasons for this to be beta deadline, though. The main reason this is targeted for a minor with a beta deadline is that it adds functionality Drupal did not previously support. It could conceivably be marked as a feature request. Not in a bad way; BigPipe itself is a great feature, and adding functionality that unblocks contrib or experimental modules is a win.

I don't actually think this going into 8.3.x only (as soon as it is ready, which is probably soon indeed) impedes innovation or BigPipe's path to stability. Sites that are running BigPipe in 8.2.x are early adopters choosing the risks of experimental modules regardless, and so they can choose to run a couple patches on their sites too if they want this functionality, especially since those patches will have already been committed to the next branch. BigPipe can still be beta now, and could even be marked as stable in 8.3.x (with this feature included) as soon as (say) the end of October.

Trying to understand the problem and the implications. Some random notes:

• StreamedResponse in the Symfony issue queue
• Original PR with lots of discussion
• The issue summary suggests that a user without a session potentially needs to wait a long time on a Varnish MISS. I see that BigPipe/StreamedResponse could reduce the time-to-first-byte. However, I do not understand how the internal page cache could help in this situation. If you have Varnish in front of your servers you have the internal page cache disabled anyway.
• I feel that people may use streamed responses to actually stream responses which are too big to fit into RAM. See, e.g. the readfile example in the original PR. I wouldn't want to store that in the internal page cache

Also:

+++ b/core/modules/page_cache/tests/modules/page_cache_test/src/PageCacheTestCacheableStreamedResponse.php
@@ -0,0 +1,26 @@
+class PageCacheTestCacheableStreamedResponse extends Response implements CacheableStreamedResponseInterface, CacheableResponseInterface {
...
+  public function __construct($streamed_response) {
+    $this->streamedResponse = $streamed_response;
+    parent::__construct('', 200, []);
+  }

What happens if the internal page cache module is uninstalled? If I understand correctly, then there is nobody left who cares to retrieve the inner streamed response from the wrapper (i.e., getCacheableStreamedResponse() is called nowhere outside the page cache module). I'd expect that the user agent will receive an empty 200 in this case?

I really dislike the convoluted logic here. Also I fear that the fake response wrapping the real response is prone to cause problems in the future.

Did you try to just implement the terminate event in BigPipe and store the response from there? On subsequent requests the internal page cache module will happily deliver the stored response. For a first iteration I suggest to just copy over the relevant bits from the page cache middleware.

Also to answer the Varnish bit:

- The streamed response we store in page cache is the same as what we send to the client. There are no JS placeholders, just chunks of data we stream. That is very important.

Varnish has perfect support in Varnish 4 (now 2 years released) for streamed responses.

It will stream the BigPipe response concurrently to all 1..n clients requesting the same URL and afterwards deliver it from its cache.

So _everyone_ is getting a fast streamed response!

So Varnish already does what we want to do in PageCache.

Stream the response and then store it in the cache.

@Wim Leers
Can you point to the upstream issue where you describe the pain points you have? Is there anything we can do about that? Can upstream help us? Please don't just complain but actually try to collaborate with Symfony. They are by orders of magnitude more responsive than on the average Drupal issue. They have opinions, much like we do have ours.

Does PSR-7's idea of using streams for the response and request body helps in your example?

Varnish has perfect support in Varnish 4 (now 2 years released) for streamed responses.

It will stream the BigPipe response concurrently to all 1..n clients requesting the same URL and afterwards deliver it from its cache.

That's exactly what i tried to express. But the issue summary suggests that we must modify the internal page cache in order to profit from streamed responses in Varnish which is nonsense. BigPipe is free to stream responses to Varnish right now, no matter whether there is a session open or not.

Thank you @Wim Leers for explaining that kind of problem.

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 have decided to think about this again and I think it is best to won't fix this, but just solve it for BigPipe specifically:

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

allows BigPipe to properly subclass it and override the method.

I think a sub class is best to start with, because per Wim logic (TM) BigPipe is just one use case of a cached response and we need at least 2, to consider a generic solution for core - usually.

Thanks for all your research, #28 and interesting that there are trailers. I did not know that.