Feat: support RR[>=2023.3] streamed responses#130
Conversation
|
@Baldinof are you still fully maitaining this repository? or you moved to different projects and now this is in maintenance-only mode? |
|
Hello @FluffyDiscord I'm still maintaining it, sorry if I've been late to respond! I'll have look on all PRs this week. Thanks for your contributions :) |
|
I think we should also add |
| $kernelCallbackRef = new \ReflectionFunction($kernelCallback); | ||
| $closureVars = $kernelCallbackRef->getClosureUsedVariables(); | ||
|
|
||
| $ref = new \ReflectionFunction($closureVars["callback"]); |
There was a problem hiding this comment.
looks like you have to have a very precise setup of callbacks used variables: $callback, $request and $requestStack. In addition to that there must be a specific instances and a provided callback must return some sort of iterable type.
Ideally, symfony's StreamedResponse callback should be compatible with response example at RR docs . But since symfony streamed response defines callback as userland function without any return - such implementation with hardcoded variable values adds coupling with RR as web runner.
There are multiple ways on how to create a Symfony's StreamedResponse, but I cannot think of possible implementation on how to transform them to generators in an elegant way without coupling and hardcoding precise structure of variables..
There was a problem hiding this comment.
This very specific setup is because while user passes callback/Closure to the StreamedResponse, Symfony's kernel wraps that up with it's own thing, and we need to know if the wrapped callback is generator or not. There is not a better way to make this more transparent to the user than this. You would need to create bundle specific streamed response. I want these PR changes to be as plug&play as possible.
The examples from that article do not make sense - why would you stream file using StreamedResponse when you can just pass it to BinaryFileResponse and you get the benefit of partial downloads too?
The easy way would be to simply pass the stream to the callback, read it by X about of bytes and yield them. Example from the article:
$stream = getMyStream();
return new StreamedResponse(
function () use ($stream) {
while (! feof($stream)) {
// echo fread($stream, 1024);
yield fread($stream, 1024); // thats it, nothing else to change
}
fclose($stream);
},
Response::HTTP_OK,
[
'Content-Transfer-Encoding', 'binary',
'Content-Type' => 'image/jpeg',
'Content-Disposition' => 'attachment; filename="attachment.jpg"',
'Content-Length' => fstat($stream)['size'],
]
);
There was a problem hiding this comment.
how to write functional tests then? Symfony wont print yielded values from function.
Leaving echo together with yield is also not an option since it will store the data to memory (ob_start(), ob_get_clean())
There was a problem hiding this comment.
I don't think I understand your issue - what do you mean by Symfony wont print yielded values from function. Just consume the generator - foreach or one of the generator functions, generator is basically an Iterable
There was a problem hiding this comment.
I mean that your provided example with yield fread($stream, 1024); won't work in a non RR environment. In other words you are breaking StreamedResponse callback's contract
You are coupling you code implementation to RoadRunner and loosing flexibility which runtime provides
There was a problem hiding this comment.
Ah, yes. Still, your argument loses meaning:
I said before, you would either need to use custom StreamedRespose that this bundle would provide or use Symfony's one and change echo's to yields.
Both changes make you runtime dependant.
By using runtime of your choice, you are immediately being "held hostage" by it, no matter which one it is. Doesn't matter if it's the default one, RR, Swoole or other variants, you will always need to adjust your code in some way, if it's running in worker mode.
Rather than arguing with me about the issue (which I am aware of), what about providing us with a solution? :)
There was a problem hiding this comment.
I do agree that it add a little coupling with RR, but the user would be fully advised, as using yield in Symfony StreamedResponse don't work at all.
As soon as it's well documented, and how it should be reverted back when removing this bundle, I'm ok with it.
There was a problem hiding this comment.
Also, maybe Symfony would accept a PR that allows to pass callback that returns generators to StreamedResponse, if you have time for another PR :)
There was a problem hiding this comment.
I just had some afterthoughts, in the end I think I would prefer to have a StreamedResponse in this bundle to handle streaming via generators.
By doing so, if a user removes this package, and was using this feature, static analysis will fail, saying class Baldinof\RoadRunnerBundle\Http\StreamedResponse does not exists. They will have a direct feedback on where they should change code to get it back to regular StreamedResponse.
Also It will be easier to maintains compatibility when not running with RR, we can just override sendContent() to consumes the generator and echo it.
Sorry for the back and forth 😅
What do you think?
There was a problem hiding this comment.
Sure. I will look at this tomorrow. Do you also want one for the rest of them, or just this one?
Baldinof
left a comment
There was a problem hiding this comment.
Thank you for the contribution, here some comments.
Once we agree on the solution, I'd like to have some tests to assure, this is working as expected.
| { | ||
| return static function () use ($response) { | ||
| $ref = new \ReflectionClass($response); | ||
| $maxlen = $ref->getProperty("maxlen")->getValue($response); |
There was a problem hiding this comment.
If possible I'd like to avoid reflexion because it breaks static analysis.
Luckily PHPStan is awesome and understands closure binding, see https://phpstan.org/r/eaa5b9fe-836e-41e5-9681-d4b8b2d062a0
Which means that we should be able to do something like
[$maxlen, $offset, $chunkSize, $deleteFileAfterSend] = Closure::bind(fn(BinaryFileResponse $r) => [
$r->maxlen,
$r->offset,
$r->chunkSize,
$r->deleteFileAfterSend
], null, BinaryFileResponse::class)($response);I have no idea of the perf impact, maybe we should cache the result of Closure::bind() into a class variable.
There was a problem hiding this comment.
The closure binding is something that I have yet to use, since I always reach for reflections. I will have to test them both and see if there's a major difference.
There was a problem hiding this comment.
I did some testing and found out that it does not matter at all if we use Reflection or Closure::bind(), at least in the context of PHP 8.2 that I used. The first instance is always slow, but the the PHP JIT cache kicks in and following ones are pretty fast and since we use workers, the PHP JIT cache will remaing through requests.
X = iteration
Y - time in MS
Cached reflection, only accessing the properties

| */ | ||
| private function createFileStreamGenerator(BinaryFileResponse $response): \Closure | ||
| { | ||
| return static function () use ($response) { |
There was a problem hiding this comment.
Why is the extra callback needed?
There was a problem hiding this comment.
I guess it's not, since every matched option returns generator anyway. Will fix
|
|
||
| $file = fopen($response->getFile()->getPathname(), "r"); | ||
|
|
||
| ignore_user_abort(true); |
There was a problem hiding this comment.
I think the return value should be stored and send back to another ignore_user_abort() call in the finally block to restore it to the previous value.
(honestly I don't even think ignore_user_abort has an impact in RoadRunner context)
There was a problem hiding this comment.
I don't think either. It was simply copied along the rest of the code from the original BinaryFileResponse. I will remove it.
| $kernelCallbackRef = new \ReflectionFunction($kernelCallback); | ||
| $closureVars = $kernelCallbackRef->getClosureUsedVariables(); | ||
|
|
||
| $ref = new \ReflectionFunction($closureVars["callback"]); |
There was a problem hiding this comment.
I think we should be able to find a solution with reflection otherwise it could starts to fail without notice due to symfony internal refactoring.
I think we can access the original callback with a listener on the kernel.response event. In the listener we would store in a weakmap, the callback associated to a response object, and here we would be able to retrieve it.
The listener could also do a check of RR_MODE and convert the callback to echo the generator, so the app can still be run with RR, or anything else (while the bundle is still in use).
What do you think?
There was a problem hiding this comment.
Your idea isn't a bad solution, but the likelihood of them refactoring this part of code or changing listener priorities to mess up your idea of the implementation will be probably the same. Don't forget tho, that these events can be stopPropagation() and then we will have to fall back to some other method of getting the callback - also low likelihood but the possibility is still there. This is really up to you.
PS: why would RR_MODE matter? if it's anything else then http, the listener will not be triggered anyway AFAIK
There was a problem hiding this comment.
I think it will depends on what we do here: #130 (comment)
If we add a custom StreamedResponse we can just extend the regular Response and Symfony will not call setCallback() to wrap it.
There was a problem hiding this comment.
RR_MODE or any other var that indicate we are not running with RR and should consume the generator and echo it
| $kernelCallbackRef = new \ReflectionFunction($kernelCallback); | ||
| $closureVars = $kernelCallbackRef->getClosureUsedVariables(); | ||
|
|
||
| $ref = new \ReflectionFunction($closureVars["callback"]); |
There was a problem hiding this comment.
I do agree that it add a little coupling with RR, but the user would be fully advised, as using yield in Symfony StreamedResponse don't work at all.
As soon as it's well documented, and how it should be reverted back when removing this bundle, I'm ok with it.
| { | ||
| $ref = new \ReflectionClass($response); | ||
|
|
||
| $encodingOptions = $ref->getProperty("encodingOptions")->getValue($response); |
There was a problem hiding this comment.
Same here, I would prefere closure binding to help static analysis.
- feat: remove callbacks - feat: replace Reflections with Closure binds
| } | ||
| } | ||
|
|
||
| public function setCallback(\Generator $callback): static |
There was a problem hiding this comment.
I think this should still accept a callable, then in the sendContent we can test if the callback returned a Generator or nothing. And if so consume and echo the generator
|
Any news here @FluffyDiscord Do you want me to take this PR over? |
Hi and sorry, I kind of forgot about this since I am using my fork. I think this should be it, for the changes. Now only tests should be missing. Let me know if theres anything else to do before the tests. |
|
Hey @FluffyDiscord / @Baldinof , any news on this? |
I have abandoned this package and made one myself. This PR can be either closed or finished by @Baldinof if he wants to. |
|
@FluffyDiscord Alright, I'll probably be looking into this in a few weeks in case you don't wanna take over @Baldinof . I can't promise anything yet, though |
|
This issue has not seen any work for a while. Since I needed it now, I'm using my own It's a custom class that extends the normal Ideally the proper solution is a fully transparent one, but that might also need changes on the Symfony side. |



Fix #101
Added support for streaming for:
Symfony\Component\HttpFoundation\StreamedJsonResponseSymfony\Component\HttpFoundation\StreamedResponse(partial, info bellow)Symfony\Component\HttpFoundation\BinaryFileResponseIn this PR the
StreamedResponsestill loads eveything in memory if the providedcallbackis not a generator. The only solution to be fully compatible would be to save the content to temp file and then stream that file. Of course, now the issue could running out of space on storage, the read -> save -> read -> send overhead and waiting basically until the original callback finished and getting only half of the streamed response feature set.More info here: https://roadrunner.dev/docs/http-resp-streaming/current/en
This is a breaking change for anyone using old RR, as they need to update the binary. Nothing else should be needed.