Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StreamedResponse not streaming with swoole integration, when using a symfony/http-kernel-based application #115

Closed
Ocramius opened this issue Feb 14, 2022 · 12 comments · Fixed by #116

Comments

@Ocramius
Copy link
Contributor

Ocramius commented Feb 14, 2022

While investigating the suitability of runtime/swoole:0.3.0, I ran into a blocker caused by streamed responses.

I was evaluating runtime/swoole because I ran into a problem similar to #50, and as of roadrunner-server/roadrunner#923 (comment), I also couldn't use RoadRunner (which I much prefer to Swoole, at first glance).

A controller producing large, slow responses

Testing this in automation is a bit of a mess (suggestions welcome though), but the idea is that I can stream a response as follows:

final class MyGiantCsvDownloadController
{
    public function someAction(): Response
    {
        return new StreamedResponse(function () {
            foreach (range(1, 5) as $i) {
                echo str_repeat((string) $i, 100000), "\n";
                sleep(1);
            }
        }, Response::HTTP_OK, [
            'Content-Type' => 'text/plain',
        ]);
    }
}

First problem: Symfony\Component\HttpKernel\EventListener\StreamedResponseListener

When interacting with this controller, through runtime/swoole, the respone is not sent via \Runtime\Swoole\SymfonyHttpBridge, where it should happen:

case $sfResponse instanceof StreamedResponse:
ob_start(function ($buffer) use ($response) {
$response->write($buffer);
return '';
});
$sfResponse->sendContent();
ob_end_clean();
$response->end();
break;

In this block, $response->write($buffer); should receive a massive chunk of 111111...22222...<SNIP>...55555, but instead, all output is sent to STDOUT in the worker (not to the response object), and a warning is produced:

<SNIP>55555555555
PHP Warning:  Swoole\Http\Response::write(): data to send is empty in /srv/share/vendor/runtime/swoole/src/SymfonyHttpBridge.php on line 50

Effectively, this write produces nothing, because the response was already sent by symfony/http-kernel:

$response->write($buffer);

After some investigation, I found that the culprit is that symfony/http-kernel sends StreamedResponse through a listener:

https://github.com/symfony/symfony/blob/82e8d23788940421e0ad6e30163242db3ba27a02/src/Symfony/Component/HttpKernel/EventListener/StreamedResponseListener.php#L27-L51

I disabled this listener by monkey-patching (while trying out stuff), but if you know a "clean" way to remove it completely from my application, lemme know.

Second problem: response buffer is completely sent in one shot

Assuming we disabled the StreamedResponseListener (clean solution pending), the response content now makes it to Swoole\Http\Response#write($buffer);, but in one big chunk: the HTTP client sees the first byte when swoole finished collecting the whole response.

This is because of this ob_start() call not having a defined buffer size specified:

ob_start(function ($buffer) use ($response) {
$response->write($buffer);
return '';
});

According to the documentation for ob_start():

If the optional parameter chunk_size is passed, the buffer will be flushed after any output call which causes the buffer's length to equal or exceed chunk_size. The default value 0 means that the output function will only be called when the output buffer is closed.

Given that PHP uses a default buffer size of 4096, perhaps it would be a good idea to add one here too? I tried it locally, and my HTTP client starts receiving bytes much earlier than 5 seconds, this way :)

@Ocramius
Copy link
Contributor Author

Ocramius commented Feb 14, 2022

My approach to disable the StreamedResponseListener in my app: replacing the service overall.

    # Disables symfony's internal response streaming, allowing for downstream
    # consumers of the HTTP kernel to stream the response as they prefer instead.
    streamed_response_listener: '@Core\Infrastructure\Symfony\HttpKernel\EventListener\DisableStreamedResponseListener'
    Core\Infrastructure\Symfony\HttpKernel\EventListener\DisableStreamedResponseListener:
        decorates: streamed_response_listener
<?php

declare(strict_types=1);

namespace Core\Infrastructure\Symfony\HttpKernel\EventListener;

use Symfony\Component\HttpKernel\EventListener\StreamedResponseListener;

/**
 * The original {@see StreamedResponseListener} takes the response during kernel
 * dispatch, and starts writing to the output buffer.
 *
 * When using `ext-openswoole`, we do not want this behavior, as it breaks streaming
 * of data between OpenSwoole workers, and the main OpenSwoole server.
 *
 * This decorator exists so it can be used to replace the behavior of the
 * {@see StreamedResponseListener}, when configured within the DIC
 */
final class DisableStreamedResponseListener extends StreamedResponseListener
{
    /**
     * Disables all events we're listening to, by design
     */
    public static function getSubscribedEvents(): array
    {
        return [];
    }
}

Ocramius added a commit to Ocramius/runtime that referenced this issue Feb 14, 2022
…ng data to swoole worker output

This change ensures that, when converting a `StreamedResponse` to output within `php-runtime/swoole`,
the output is streamed chunk-by-chunk, instead of being buffered in its entirety, and then being
sent out to the swooler server all in one shot.

This should fix some obvious performance, latency and memory issues related to streaming common responses
assembled by PHP-side processes.

Note: you will still need to disable symfony's `StreamedResponseListener` when working with this package.

Ref: php-runtime#115
Ocramius added a commit to Ocramius/runtime that referenced this issue Feb 14, 2022
…ng data to swoole worker output

This change ensures that, when converting a `StreamedResponse` to output within `php-runtime/swoole`,
the output is streamed chunk-by-chunk, instead of being buffered in its entirety, and then being
sent out to the swooler server all in one shot.

This should fix some obvious performance, latency and memory issues related to streaming common responses
assembled by PHP-side processes.

Note: you will still need to disable symfony's `StreamedResponseListener` when working with this package.

Ref: php-runtime#115
@Ocramius
Copy link
Contributor Author

Potential fix at #116

@alexander-schranz
Copy link
Member

alexander-schranz commented Feb 14, 2022

The StreamedResponseListener is very strange. If I'm analysing the case when symfony response listener was added in the area between symfony 2.0 and symfony 2.1 it seems like in that area the front controller was not yet sending the response.

https://github.com/symfony/symfony-standard/blob/v2.0.25/web/app.php

That was changed in the next release 2.1:

https://github.com/symfony/symfony-standard/blob/v2.1.0/web/app.php

So from my point of view the StreamedResponseListener seems like something legacy which would since symfony 2.1 not needed but was never removed from symfony codebase itself. But that is just how it looks for me, maybe @Nyholm know more about it.

Current workaround could be in your project using a compilerpass in your Kernel.php removing the service so you don't need to overwrite it in your services.yaml:

class Kernel extends BaseKernel implements CompilerPassInterface
{
    public function process(ContainerBuilder $container): void
    {
        $container->removeDefinition('streamed_response_listener');
    }
}

I personally would also prefer that the service definition will be removed from symfony itself, or set it to a state where it does nothing in its default setting.

@Ocramius
Copy link
Contributor Author

seems like something legacy which would since symfony 2.1 not needed but was never removed from symfony codebase itself.

Dunno, some integration tests fail for me since I disabled it, but I'll check on my end when I have time.

@alexander-schranz
Copy link
Member

Maybe @nicolas-grekas or @fabpot could tell us about the need of the StreamedResponseListener in Symfony.

@alexander-schranz
Copy link
Member

Good news seems like it looks good the StreamedResponseListener will be removed with Symfony 6.1: symfony/symfony#45476

fabpot added a commit to symfony/symfony that referenced this issue Feb 18, 2022
…rves no purpose anymore (nicolas-grekas)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpKernel] Deprecate StreamedResponseListener, it serves no purpose anymore

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Tickets       | -
| License       | MIT
| Doc PR        | -

`StreamedResponseListener` has been introduced at the same time as `StreamedResponse` in #2935.

Its purpose was to make catching exceptions easier by wrapping the call to `$response->send()` in the main try/catch of `HttpKernel`.

Since #12998, we have `HttpKernel::terminateWithException()`, and we don't need that anymore, so we can just remove the listener.

This will help [integrate Symfony into e.g. Swoole](php-runtime/runtime#115) /cc @alexander-schranz.

Commits
-------

ee61774 [HttpKernel] Deprecate StreamedResponseListener, it serves no purpose anymore
alexander-schranz pushed a commit that referenced this issue Feb 21, 2022
…swoole worker output (#116)

* Fix #115 - buffer each `StreamedResponse` chunk when sending data to swoole worker output

This change ensures that, when converting a `StreamedResponse` to output within `php-runtime/swoole`,
the output is streamed chunk-by-chunk, instead of being buffered in its entirety, and then being
sent out to the swooler server all in one shot.

This should fix some obvious performance, latency and memory issues related to streaming common responses
assembled by PHP-side processes.

Note: you will still need to disable symfony's `StreamedResponseListener` when working with this package.

Ref: #115

* Disabled error handler replacement, which is implicitly done by the `GenericRuntime`

We don't want to replace the error handler: PHPUnit is good enough. This should bring
the tests back to green too.

* Upgrade PHPUnit configuration to comply with latest `phpunit.xsd`

Ref: https://github.com/sebastianbergmann/phpunit/blob/f61e897412fe831831462c0698a6208f7d8298f0/phpunit.xsd#L255
Ref: https://github.com/sebastianbergmann/phpunit/blob/f61e897412fe831831462c0698a6208f7d8298f0/phpunit.xsd#L16-L20
@jelovac
Copy link

jelovac commented Mar 22, 2022

My approach to disable the StreamedResponseListener in my app: replacing the service overall.

    # Disables symfony's internal response streaming, allowing for downstream
    # consumers of the HTTP kernel to stream the response as they prefer instead.
    streamed_response_listener: '@Core\Infrastructure\Symfony\HttpKernel\EventListener\DisableStreamedResponseListener'
    Core\Infrastructure\Symfony\HttpKernel\EventListener\DisableStreamedResponseListener:
        decorates: streamed_response_listener
<?php

declare(strict_types=1);

namespace Core\Infrastructure\Symfony\HttpKernel\EventListener;

use Symfony\Component\HttpKernel\EventListener\StreamedResponseListener;

/**
 * The original {@see StreamedResponseListener} takes the response during kernel
 * dispatch, and starts writing to the output buffer.
 *
 * When using `ext-openswoole`, we do not want this behavior, as it breaks streaming
 * of data between OpenSwoole workers, and the main OpenSwoole server.
 *
 * This decorator exists so it can be used to replace the behavior of the
 * {@see StreamedResponseListener}, when configured within the DIC
 */
final class DisableStreamedResponseListener extends StreamedResponseListener
{
    /**
     * Disables all events we're listening to, by design
     */
    public static function getSubscribedEvents(): array
    {
        return [];
    }
}

@Ocramius thanks for the workaround. You have literally saved me hours on troubleshooting why returning a file from an object storage via streamed response was malformed under ApiPlatform running via RoadRunner (runtime/roadrunner-symfony-nyholm).

I saw your issue roadrunner-server/roadrunner#923 which lead me to this one.

Are there any side effects by disabling the listener?

@Ocramius
Copy link
Contributor Author

So far, none that I could notice

@alexander-schranz
Copy link
Member

@jelovac they removed the listener also in symfony core for newer versions: symfony/symfony#45476
so does not have any effects.

@jelovac
Copy link

jelovac commented Mar 22, 2022

@jelovac they removed the listener also in symfony core for newer versions: symfony/symfony#45476 so does not have any effects.

It seems that I have found one side effect.

By disabling StreamedResponseListener integration tests which fetched streamed response are now failing. I think this has something to do with how phpunit bootstraps ApiPlatform application when performing integration tests using their src/Bridge/Symfony/Bundle/Test/ApiTestCase.php class. Will investigate tomorrow further.

@jelovac
Copy link

jelovac commented Mar 23, 2022

@jelovac they removed the listener also in symfony core for newer versions: symfony/symfony#45476 so does not have any effects.

It seems that I have found one side effect.

By disabling StreamedResponseListener integration tests which fetched streamed response are now failing. I think this has something to do with how phpunit bootstraps ApiPlatform application when performing integration tests using their src/Bridge/Symfony/Bundle/Test/ApiTestCase.php class. Will investigate tomorrow further.

Couldn't figure it out so I added a workaround for the test environment based on your example:

    public function process(ContainerBuilder $container): void
    {
        if ('test' !== $this->environment) {
            $container->removeDefinition('streamed_response_listener');
        }
    }

This now satisfies both use cases.

Lets hope this won't haunt me :)

@Deltachaos
Copy link

Deltachaos commented May 11, 2022

I have been playing around with the Symfony StreamedResponse as well and tried to find a solution for the problem with the buffer size that @Ocramius is describing as well. In my opinion a simple and BC friendly solution would be to pass a stream target as first parameter to the callback of the Symfony response so that it becomes something like this:

 $response = new StreamedResponse();
 $response->setCallback(function ($target) use($iterator) {
     $handle = fopen($target, 'r+');
     foreach ($iterator as $data) {
         fwrite($handle, $data);
     }
     fclose($handle);
});

This would also make async responses possible:

 $response = new StreamedResponse();
 $response->setCallback(function ($target) use($promise, $response) {
     $handle = fopen($target, 'r+');
     $promise->then(function($iterator) {
         foreach ($iterator as $data) {
             fwrite($handle, $data);
         }
         fclose($handle);
     }, function($error) {
         fclose($handle);
     });
});

Using this approach the runtime could create a streamWrapper and pass this as a target. The Symfony default could simply be `php://output`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants