symfony / symfony

The Symfony PHP framework
https://symfony.com
MIT License
29.75k stars 9.46k forks source link

Automatic content length in Reponse::send() can break some responses #46449

Closed catch56 closed 2 years ago

catch56 commented 2 years ago

Symfony version(s) affected

6.1

Description

Drupal's Big Pipe module does some custom chunking of responses.

After https://github.com/symfony/http-foundation/commit/38a3244eb0c33fbb9ce87da1677cbbde488c3750 a content length is automatically added to the response, which breaks the page.

We can workaround this by doing an explicit $this->headers->set('Transfer-Encoding', ''); and might be able to come up with something better in the future, however thought it was worth opening an issue just to document the behaviour change, since it's not impossible other projects are doing something weird like us.

How to reproduce

More discussion on the Drupal.org issue https://www.drupal.org/project/drupal/issues/3276186

To reproduce, install Drupal core, update Symfony to 6.1, enable big pipe module.

Possible Solution

No response

Additional Context

No response

nicolas-grekas commented 2 years ago

That's strange indeed, because we also check that \is_string($this->content) && '' !== $this->content before sending the header. Why chunking if the content is already buffered?

nicolas-grekas commented 2 years ago

$this->headers->set('Transfer-Encoding', '');

Alternatively, you might want to call $this->sendHeaders(); and $this->sendContent(); separately?

stof commented 2 years ago

@catch56 if you send content in a special way, BigPipeResponse should probably set false as the buffered content of the response instead of setting the unprocessed content.

qdequippe commented 2 years ago

I have maybe related (or same) problem after upgrading my Symfony project to 6.1. I got net::ERR_HTTP2_PROTOCOL_ERROR 200 error in my browser console (Chrome). It appears on the homepage on my website, only a part of the page is rendered.

Using workaround $response->headers->set('Transfer-Encoding', ''); fixed the error.

nicolas-grekas commented 2 years ago

Can you figure out why you're having this issue @qdequippe? That might be useful to decide what we should do here.

qdequippe commented 2 years ago

@nicolas-grekas my skills are weak on the subject but I am using Apache as a web server with mod_deflate activated so maybe it's related to "compression" (I saw issues related to "double compression") ...

Also I don't have any problems on my local web server (Symfony Local Web Server with CLI).

nicolas-grekas commented 2 years ago

Can you share a small Symfony app that exhibits this behavior behind apache?

qdequippe commented 2 years ago

@nicolas-grekas I tried to reproduce it locally but without success. It's a closed source project but I open a temporary "test" area if you need here https://test.lecosinus.fr/fr/ I hope it can help.

apollisa commented 2 years ago

I also have a problem with this change, but in a different way than what @qdequippe and @catch56 reported: the content length reported in the header is sometimes much greater than what is effectively sent, and this causes the browser (both Firefox and Chrome) to wait for about 5 s the end of the response, before giving up.

When trying the request in the JetBrains HTTP client, an error is raised which confirms the problem:

org.apache.http.ConnectionClosedException: Premature end of Content-Length delimited message body (expected: 2 201; received: 911)

You can try this out on this page: https://staging.rattra.page/login

Reverting https://github.com/symfony/http-foundation/commit/38a3244eb0c33fbb9ce87da1677cbbde488c3750 fixes this.

nicolas-grekas commented 2 years ago

We'd need a reproducing app to understand what's going on. Could any of you provide one for us?

qdequippe commented 2 years ago

@apollisa I try to reproduce the problem and I try to figure out if it comes from a specific web server, in my case Apache. Which web server are you using?

apollisa commented 2 years ago

@qdequippe Apache as well, with GZIP compression enabled, just as you. @nicolas-grekas I am currently setting up the Symfony demo app on my server, I will keep you posted. ^^

apollisa commented 2 years ago

Here you go @nicolas-grekas https://46449.rattra.page/

This is the Symfony webapp skeleton with a controller rendering a template with a test paragraph, behind OVH web cloud Apache server.

nicolas-grekas commented 2 years ago

Thanks. What's the source code of the app behind? And what's the content like when the content-length header is computed?

qdequippe commented 2 years ago

@apollisa @nicolas-grekas same test https://demo.dequippe.tech/, source code available here https://github.com/qdequippe/sfdemo but unable to reproduce locally the web server configuration because it's a shared web hosting (OVH also), even with available public configurations https://webhosting-infos.hosting.ovh.net

apollisa commented 2 years ago

@nicolas-grekas the source code is here: https://github.com/apollisa/46449

And the content when the header is computed is as follow:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title>Welcome!</title>
        <link rel="icon" href="data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 128 128%22><text y=%221.2em%22 font-size=%2296%22>⚫️</text></svg>">

            </head>
    <body>
          <p>Test</p>
    </body>
</html>
nicolas-grekas commented 2 years ago

Premature end of Content-Length delimited message body (expected: 2 201; received: 911)

That doesn't make sense to me: the page I'm seeing at https://staging.rattra.page/login is of the expected size (2201). Can you figure out when there is a mismatch between the Content-Length header and the actual length of the content?

apollisa commented 2 years ago

I believe the mismatch comes from the compression:

image

The content length is indeed different from the actual received length, because fewer bytes are sent, thanks to compression. Does that make sense?

stof commented 2 years ago

According to the discussion in https://github.com/e107inc/e107/issues/2638 and https://bugs.php.net/bug.php?id=44164, this might happen if you use zlib.output_compression in your PHP config, applying the gzip compression in PHP instead of letting Apache handling it. Could it be the case in your shared hosting ?

qdequippe commented 2 years ago

@stof according info we have on https://phpfpm81.webhosting-infos.hosting.ovh.net output compression is off. I will open a ticket on OVH side for more details.

qdequippe commented 2 years ago

The response from web hosting provider:

I regret to inform you that your request exceeds the scope of intervention of our Support Service.

I will keep trying to reproduce it locally using a custom Docker environment...

nicolas-grekas commented 2 years ago

@catch56 I checked the patch on Drupal and I think @stof is right, this one would be better:

--- a/core/modules/big_pipe/src/Render/BigPipeResponse.php
+++ b/core/modules/big_pipe/src/Render/BigPipeResponse.php
@@ -48,6 +48,7 @@ class BigPipeResponse extends HtmlResponse {
    */
   public function __construct(HtmlResponse $response) {
     parent::__construct('', $response->getStatusCode(), []);
+    $this->content = false;

     $this->originalHtmlResponse = $response;
catch56 commented 2 years ago

@nicolas-grekas OK yeah I tried overriding ::send() and it is not a disaster but still feels like a temporary workaround. Do you think setting content to false is how it should be left permanently?

nicolas-grekas commented 2 years ago

Yes, setting content to false is the API also used by StreamedResponse for a similar purpose.

nicolas-grekas commented 2 years ago

Hum, actually StreamedResponse is doing this instead

    public function getContent()
    {
        return false;
    }

so it might be even better. (you could also extend from StreamedResponse if that'd make sense btw)

catch56 commented 2 years ago

@nicolas-grekas ah OK but this is why we have BigPipeResponse in the first place, because we actually use what's in $this->content (see Drupal\big_pipe\Render\BigPipe). StreamedResponse puts content behind a callback which doesn't work for us (or at least it didn't work for the people who implemented this a few years ago).

nicolas-grekas commented 2 years ago

Let's make it easier: I'm proposing to revert #45092 in #46523

catch56 commented 2 years ago

Just for the record we have at least two viable workarounds that do not feel permanent, but are not a disaster, however I think something (whether an interface or a flag) would be useful here to indicate that setting a content length header is desired.

nicolas-grekas commented 2 years ago

Thanks for the discussion.

wimleers commented 2 years ago

(you could also extend from StreamedResponse if that'd make sense btw)

Unfortunately we can't, because class BigPipeResponse extends HtmlResponse and class HtmlResponse extends Response implements CacheableResponseInterface, AttachmentsInterface. Not a dealbreaker yet.

The real blocker here: lots of $response instanceof HtmlResponse checks. If that were done using an interface (😬), it'd have been possible.

I vaguely recall this from when I was working on BigPipe many years ago. IIRC (Symfony's) StreamedResponse seemed semantically more distant from what BigPipeResponse actually does/contains than (Drupal's) HtmlResponse. In the end, it really still is guaranteed to be a HtmlResponse, just delivered in streaming/chunked fashion. Perhaps unfortunate today, but so far it has served us fine.