Feat: support RR[>=2023.3] streamed responses #130

FluffyDiscord · 2023-12-03T11:22:41Z

Added support for streaming for:

Symfony\Component\HttpFoundation\StreamedJsonResponse
Symfony\Component\HttpFoundation\StreamedResponse (partial, info bellow)
Symfony\Component\HttpFoundation\BinaryFileResponse

In this PR the StreamedResponse still loads eveything in memory if the provided callback is 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.

FluffyDiscord · 2023-12-03T11:23:35Z

@Baldinof are you still fully maitaining this repository? or you moved to different projects and now this is in maintenance-only mode?

Baldinof · 2023-12-03T19:53:30Z

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 :)

FluffyDiscord · 2023-12-03T20:03:51Z

I think we should also add Spiral\RoadRunner\Http\Exception\StreamStoppedException to the list of default ignored exceptions as this can cause false reports. It should be handled by the generators that are added in this PR though.

rmikalkenas · 2023-12-04T11:00:50Z

src/RoadRunnerBridge/HttpFoundationWorker.php

+ $kernelCallbackRef = new \ReflectionFunction($kernelCallback);
+ $closureVars = $kernelCallbackRef->getClosureUsedVariables();
+
+ $ref = new \ReflectionFunction($closureVars["callback"]);


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

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'], ] );

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())

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

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

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? :)

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.

Also, maybe Symfony would accept a PR that allows to pass callback that returns generators to StreamedResponse, if you have time for another PR :)

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?

Sure. I will look at this tomorrow. Do you also want one for the rest of them, or just this one?

Baldinof

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.

Baldinof · 2023-12-05T22:34:01Z

src/RoadRunnerBridge/HttpFoundationWorker.php

+ {
+ return static function () use ($response) {
+ $ref = new \ReflectionClass($response);
+ $maxlen = $ref->getProperty("maxlen")->getValue($response);


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.

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.

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

New reflection each response

Cached reflection, only accessing the properties

New Closure bind each response

Cached closure bind

Baldinof · 2023-12-05T22:34:38Z

src/RoadRunnerBridge/HttpFoundationWorker.php

+ */
+ private function createFileStreamGenerator(BinaryFileResponse $response): \Closure
+ {
+ return static function () use ($response) {


Why is the extra callback needed?

I guess it's not, since every matched option returns generator anyway. Will fix

Baldinof · 2023-12-05T22:37:22Z

src/RoadRunnerBridge/HttpFoundationWorker.php

+
+ $file = fopen($response->getFile()->getPathname(), "r");
+
+ ignore_user_abort(true);


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)

I don't think either. It was simply copied along the rest of the code from the original BinaryFileResponse. I will remove it.

Baldinof · 2023-12-06T20:56:04Z

src/RoadRunnerBridge/HttpFoundationWorker.php

+ $kernelCallbackRef = new \ReflectionFunction($kernelCallback);
+ $closureVars = $kernelCallbackRef->getClosureUsedVariables();
+
+ $ref = new \ReflectionFunction($closureVars["callback"]);


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?

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

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.

RR_MODE or any other var that indicate we are not running with RR and should consume the generator and echo it

Baldinof · 2023-12-06T20:58:55Z

src/RoadRunnerBridge/HttpFoundationWorker.php

+ $kernelCallbackRef = new \ReflectionFunction($kernelCallback);
+ $closureVars = $kernelCallbackRef->getClosureUsedVariables();
+
+ $ref = new \ReflectionFunction($closureVars["callback"]);


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.

Baldinof · 2023-12-06T21:01:15Z

src/Helpers/StreamedJsonResponseHelper.php

+ {
+ $ref = new \ReflectionClass($response);
+
+ $encodingOptions = $ref->getProperty("encodingOptions")->getValue($response);


Same here, I would prefere closure binding to help static analysis.

- feat: remove callbacks - feat: replace Reflections with Closure binds

Baldinof · 2023-12-13T22:55:06Z

src/Http/Response/StreamedResponse.php

+ }
+ }
+
+ public function setCallback(\Generator $callback): static


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

Baldinof · 2024-01-11T10:40:03Z

Any news here @FluffyDiscord Do you want me to take this PR over?

FluffyDiscord · 2024-01-11T18:15:33Z

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.

L3tum · 2024-02-26T14:52:52Z

Hey @FluffyDiscord / @Baldinof , any news on this?

FluffyDiscord · 2024-02-26T15:02:28Z

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.

L3tum · 2024-02-26T15:08:09Z

@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

- feat: support RR[>=2023.3] streamed responses

ed35dce

rmikalkenas reviewed Dec 4, 2023

View reviewed changes

- feat: mention another allowed exception

81c9d7b

Baldinof reviewed Dec 6, 2023

View reviewed changes

- feat: introduce StreamedResponse replacement

07388ec

- feat: remove callbacks - feat: replace Reflections with Closure binds

Baldinof requested changes Dec 13, 2023

View reviewed changes

- feat: allow callback too for streamed response

b1814f0

Baldinof mentioned this pull request Mar 13, 2024

feat: introduce KV cache adapters #137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: support RR[>=2023.3] streamed responses #130

Feat: support RR[>=2023.3] streamed responses #130

FluffyDiscord commented Dec 3, 2023 •

edited

FluffyDiscord commented Dec 3, 2023

Baldinof commented Dec 3, 2023

FluffyDiscord commented Dec 3, 2023

rmikalkenas Dec 4, 2023

FluffyDiscord Dec 4, 2023 •

edited

rmikalkenas Dec 4, 2023

FluffyDiscord Dec 4, 2023 •

edited

rmikalkenas Dec 5, 2023 •

edited

FluffyDiscord Dec 5, 2023 •

edited

Baldinof Dec 6, 2023

Baldinof Dec 6, 2023

Baldinof Dec 6, 2023

FluffyDiscord Dec 6, 2023

Baldinof left a comment

Baldinof Dec 5, 2023

FluffyDiscord Dec 6, 2023

FluffyDiscord Dec 8, 2023

Baldinof Dec 5, 2023

FluffyDiscord Dec 6, 2023 •

edited

Baldinof Dec 5, 2023

FluffyDiscord Dec 6, 2023

Baldinof Dec 6, 2023

FluffyDiscord Dec 6, 2023

Baldinof Dec 6, 2023

Baldinof Dec 6, 2023

Baldinof Dec 6, 2023

Baldinof Dec 6, 2023

Baldinof Dec 13, 2023

Baldinof commented Jan 11, 2024 •

edited

FluffyDiscord commented Jan 11, 2024 •

edited

L3tum commented Feb 26, 2024

FluffyDiscord commented Feb 26, 2024

L3tum commented Feb 26, 2024


		$file = fopen($response->getFile()->getPathname(), "r");

		ignore_user_abort(true);

Feat: support RR[>=2023.3] streamed responses #130

Are you sure you want to change the base?

Feat: support RR[>=2023.3] streamed responses #130

Conversation

FluffyDiscord commented Dec 3, 2023 • edited

FluffyDiscord commented Dec 3, 2023

Baldinof commented Dec 3, 2023

FluffyDiscord commented Dec 3, 2023

Choose a reason for hiding this comment

FluffyDiscord Dec 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FluffyDiscord Dec 4, 2023 • edited

Choose a reason for hiding this comment

rmikalkenas Dec 5, 2023 • edited

Choose a reason for hiding this comment

FluffyDiscord Dec 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baldinof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FluffyDiscord Dec 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baldinof commented Jan 11, 2024 • edited

FluffyDiscord commented Jan 11, 2024 • edited

L3tum commented Feb 26, 2024

FluffyDiscord commented Feb 26, 2024

L3tum commented Feb 26, 2024

FluffyDiscord commented Dec 3, 2023 •

edited

FluffyDiscord Dec 4, 2023 •

edited

FluffyDiscord Dec 4, 2023 •

edited

rmikalkenas Dec 5, 2023 •

edited

FluffyDiscord Dec 5, 2023 •

edited

FluffyDiscord Dec 6, 2023 •

edited

Baldinof commented Jan 11, 2024 •

edited

FluffyDiscord commented Jan 11, 2024 •

edited