php-runtime / runtime

A home for runtimes.
MIT License
396 stars 31 forks source link

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

Closed Ocramius closed 2 years ago

Ocramius commented 2 years ago

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 https://github.com/php-runtime/runtime/issues/50, and as of https://github.com/roadrunner-server/roadrunner/issues/923#issuecomment-817113479, 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:

https://github.com/php-runtime/runtime/blob/420d39e651580bd11904655dec3c7d3411a6f6c2/src/swoole/src/SymfonyHttpBridge.php#L48-L57

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:

https://github.com/php-runtime/runtime/blob/420d39e651580bd11904655dec3c7d3411a6f6c2/src/swoole/src/SymfonyHttpBridge.php#L50

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:

https://github.com/php-runtime/runtime/blob/420d39e651580bd11904655dec3c7d3411a6f6c2/src/swoole/src/SymfonyHttpBridge.php#L49-L53

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 commented 2 years ago

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 commented 2 years ago

Potential fix at #116

alexander-schranz commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

alexander-schranz commented 2 years ago

Good news seems like it looks good the StreamedResponseListener will be removed with Symfony 6.1: https://github.com/symfony/symfony/pull/45476

jelovac commented 2 years ago

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 https://github.com/roadrunner-server/roadrunner/issues/923 which lead me to this one.

Are there any side effects by disabling the listener?

Ocramius commented 2 years ago

So far, none that I could notice

alexander-schranz commented 2 years ago

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

jelovac commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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`.