`StreamedResponse` not streaming with swoole integration, when using a `symfony/http-kernel`-based application
Ocramius opened this issue · 12 comments
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:
runtime/src/swoole/src/SymfonyHttpBridge.php
Lines 48 to 57 in 420d39e
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:
After some investigation, I found that the culprit is that symfony/http-kernel sends StreamedResponse through a listener:
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:
runtime/src/swoole/src/SymfonyHttpBridge.php
Lines 49 to 53 in 420d39e
According to the documentation for ob_start():
If the optional parameter
chunk_sizeis passed, the buffer will be flushed after any output call which causes the buffer's length to equal or exceedchunk_size. The default value0means 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 :)
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 [];
}
}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.
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.
Maybe @nicolas-grekas or @fabpot could tell us about the need of the StreamedResponseListener in Symfony.
Good news seems like it looks good the StreamedResponseListener will be removed with Symfony 6.1: symfony/symfony#45476
My approach to disable the
StreamedResponseListenerin 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?
So far, none that I could notice
@jelovac they removed the listener also in symfony core for newer versions: symfony/symfony#45476
so does not have any effects.
@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 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 :)
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`.