Skip to content

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

Merged
alexander-schranz merged 3 commits into
php-runtime:mainfrom
Ocramius:fix/#115-ensure-symfony-streamed-response-is-actually-streamed-to-swoole
Feb 21, 2022
Merged

alexander-schranz merged 3 commits into
php-runtime:mainfrom
Ocramius:fix/#115-ensure-symfony-streamed-response-is-actually-streamed-to-swoole

Conversation

@Ocramius

@Ocramius Ocramius commented Feb 14, 2022 •

Copy link
Copy Markdown
Contributor

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.
Comment thread src/swoole/tests/Unit/RuntimeTest.php Resolved
@Ocramius

Ocramius commented Feb 14, 2022 •

Copy link
Copy Markdown
Contributor Author

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

@alexander-schranz alexander-schranz left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@Ocramius

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Thanks @alexander-schranz!

@Ocramius

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@Ocramius

Copy link
Copy Markdown
Contributor Author

Awesome, thanks!

forfin pushed a commit to forfin/runtime that referenced this pull request Dec 27, 2025
…ng data to swoole worker output (php-runtime#116)

* Fix php-runtime#115 - buffer each `StreamedResponse` chunk when sending 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

* Disabled error handler replacement, which is implicitly done by the `GenericRuntime`

We don't want to replace the error handler: PHPUnit is good enough. This should bring
the tests back to green too.

* Upgrade PHPUnit configuration to comply with latest `phpunit.xsd`

Ref: https://github.com/sebastianbergmann/phpunit/blob/f61e897412fe831831462c0698a6208f7d8298f0/phpunit.xsd#L255
Ref: https://github.com/sebastianbergmann/phpunit/blob/f61e897412fe831831462c0698a6208f7d8298f0/phpunit.xsd#L16-L20
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

⌂ Proxy