Skip to content

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

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

Conversation

Ocramius
Copy link
Contributor

@Ocramius Ocramius commented Feb 14, 2022

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.

Fixes #115

…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 Ocramius force-pushed the fix/#115-ensure-symfony-streamed-response-is-actually-streamed-to-swoole branch 2 times, most recently from 9aa355f to a19eccb Compare February 14, 2022 15:35
…GenericRuntime`

We don't want to replace the error handler: PHPUnit is good enough. This should bring
the tests back to green too.
@Ocramius
Copy link
Contributor Author

Ocramius commented Feb 14, 2022

The last deprecation that seems to make CI fail is caused by Symfony\Component\HttpFoundation#get() being called by Illuminate\Http\Request due to inheritance.

Because the parent class != child class, this code leads to a deprecation error:

        if (!$dup->get('_format') && $this->get('_format')) {
            $dup->attributes->set('_format', $this->get('_format'));
        }

Overall, that's not fixable in this library.

EDIT: suggestion: disable symfony's deprecation system, and just use phpunit/phpunit, which would've saved ~1h of work on my end :P

@Ocramius Ocramius force-pushed the fix/#115-ensure-symfony-streamed-response-is-actually-streamed-to-swoole branch from 6e844a4 to 5be78a2 Compare February 14, 2022 16:27
Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks fine from my point of view. But I'm not familiar enough with the error_handler. But it seems only effecting phpunit tests not application itself?

The tests deprecations which are thrown in the CI were also before there so not related to your PR changes.

@Ocramius
Copy link
Contributor Author

But it seems only effecting phpunit tests not application itself?

Yes, that's because of the runner modifying global state in an unclean way. TBH, reminds me that I have to disable that error handler for my own use-cases as well :)

@Ocramius
Copy link
Contributor Author

Going to release this on a fork meanwhile, to allow for local usage on my end.

@Ocramius
Copy link
Contributor Author

@alexander-schranz I see this is approved - anything else to be done to get it through, to a release?

@alexander-schranz alexander-schranz merged commit d2a9d3e into php-runtime:main Feb 21, 2022
@alexander-schranz
Copy link
Member

@Ocramius Sorry for the delay. Did work all like expected, Thank you for the pull request.
From my side there is nothing which is against about releasing this. /cc @Nyholm

@alexander-schranz alexander-schranz added the bug Something isn't working label Feb 21, 2022
@Ocramius Ocramius deleted the fix/#115-ensure-symfony-streamed-response-is-actually-streamed-to-swoole branch February 21, 2022 11:10
@Ocramius
Copy link
Contributor Author

Thanks @alexander-schranz!

@Ocramius
Copy link
Contributor Author

Is some automation needed to cut 0.3.1 or such? Looking at https://github.com/php-runtime/swoole/releases

@alexander-schranz
Copy link
Member

@Ocramius Was missing some permissions. Now 0.3.1 is tagged!

@Ocramius
Copy link
Contributor Author

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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