Add support for StreamedResponse

[supersmile2009]

This PR adds support for StreamedResponse by making use of PHP's output buffering functions.

It's designed to behave as close as possible to original StreamedResponse handling when running Symfony with php-fpm. Compiler pass replaces Symfony's Symfony\Component\HttpKernel\EventListener\StreamedResponseListener with a custom listener, which triggers returning response to the client. The reason listener is used instead of simply calling the response processor from the HttpKernelRequestHandler is the same as originally in Symfony - a callback in StreamedResponse may depend on some services, which depend on Request context (current Request being present in RequestStack). Also error handling that you have in your Symfony app can kick in if the callback fails, which is not the case if we process the StreamedResponse outside of the request context.

Before I start writing tests, please let me know first if the overall design of this solution looks good to you, or you'd rather take some different approach.

[k911]

Thanks @supersmile2009 for PR, overall I think this is a really good feature, but I'll have to take more time to review it properly because I do not like the idea of keeping swoole request/response objects in symfony's request/response objects.

Obviously, you can add general feature tests, which should be working anyway, even with a little bit different implementation of this feature

[supersmile2009]

@k911 what about creating SwooleContext service - a simple service to contain Swoole's Request and Response objects? We could set both request and response into it from HttpKernelRequestHandler. Then this Context could be injected into StreamedResponseListener (or any other service in future if anyone needs original Swoole's request/response for any reason).
SwooleContext could implement Symfony\Contracts\Service\ResetInterface to clean up state.
The downside of this solution obviously is SwooleContext being stateful.

To be honest I'm not a big fan of either of these solutions. But at least with the current implementation with the use of request attributes there are not stateful services, so no reliance on ResetInterface and no potential side-effects. And all interaction with the attributes is done through the SwooleRequestResponseContextManager, so attribute names are encapsulated.

[k911]

Anyway @supersmile2009 why not just process this response in src/Bridge/Symfony/HttpFoundation/ResponseProcessor.php directly in src/Bridge/Symfony/HttpKernel/HttpKernelRequestHandler.php and just create NoOpStreamedResponseListener or disable it in container?

[k911]

@supersmile2009 Please check out this approach https://github.com/supersmile2009/swoole-bundle/pull/1/files (Rebase your PR onto develop, to get clean changes)

[supersmile2009]

Anyway @supersmile2009 why not just process this response in src/Bridge/Symfony/HttpFoundation/ResponseProcessor.php directly in src/Bridge/Symfony/HttpKernel/HttpKernelRequestHandler.php and just create NoOpStreamedResponseListener or disable it in container?

Same reason as why Symfony has StreamedResponseListener instead of sending response in index.php like it does for other types of responses. I briefly explained it in the PR description:

a callback in StreamedResponse may depend on some services, which depend on Request context (current Request being present in RequestStack). Also error handling that you have in your Symfony app can kick in if the callback fails, which is not the case if we process the StreamedResponse outside of the request context.

I wouldn't want to complicate it, but this is the way this feature was designed in Symfony, so I'm just trying to make it work the same way with Swoole.
I wouldn't say that returning StreamedResponse depending on RequestStack (directly or indirectly) is a good design choice, so I don't really care much about that aspect. But error handling is a bigger deal. If we don't add a listener and simply process the response in src/Bridge/Symfony/HttpFoundation/ResponseProcessor.php, then in case of exception listeners of kernel.exception won't be able to do their job. That's exactly the reason why Symfony has Symfony\Component\HttpKernel\EventListener\StreamedResponseListener.

One of the changes that I did which I didn't really like was this piece in K911\Swoole\Bridge\Symfony\HttpKernel\HttpKernelRequestHandler

        if (!$httpFoundationResponse instanceof StreamedResponse) {
            $this->responseProcessor->process($httpFoundationResponse, $response);
        }

Reasons:

• What if anyone needs to add any other type of response to the exclusion list. E. g. someone needs to write to Swoole's Response using some other listener. Overall this solution is not very extensible.
• Also it's tightly and at the same time indirectly coupled to the existence of the StreamedResponseListener.
• Differentiating between response types shouldn't be a responsibility of the request handler.

So I updated that part. Please have a look at the last 2 commits. Now request processors are a stack of decorators.
HttpKernelRequestHandler gets a stack with NoOpStreamedResponseProcessor on top, which achieves the same goal as that if statement.
StreamedResponseListener gets a proper StreamedResponseProcessor in the stack.

[k911]

Thanks @supersmile2009 for explanation, let's go with this approach. Let me know if you'll have some troubles with unit testing / feature tests so we can get this merged

[k911]

@supersmile2009 I just wanted to kindly remind you of this PR, please tell me if you need any help or don't want to continue your work.

[supersmile2009]

Hi @k911
Thanks for the reminder. I haven't had a chance to work on it lately. But I still have intention to finish it. I'm quite busy preparing for a big release at my current job right now. But I'll get back to this PR by mid September, because we want to get rid of the patch and have it merged to upstream :)

[k911]

@supersmile2009 Great, thanks for an update! :)

PS. Good luck with the release :)

[supersmile2009]

@k911 PR is updated with all the requested changes.

[k911]

Hi, @supersmile2009 thanks. Unfortunately CI and some tests failed. I'll take a look at this closer probably next week, however feel free to update PR, too - if you know what's going on.

[Webonaute]

anything I can help with to make this release?

[k911]

Hi @Webonaute, we need to resolve problems that occurred during CI build, which are Unit tests /code style issues, then make sure we tested everything properly via feature tests. I think that if @supersmile2009 won't be able to finish work here, you could fork his branch and fix that issues.

[codecov]

Codecov Report

Merging #300 (6e495e2) into develop (807ba9f) will decrease coverage by 0.03%.
The diff coverage is 86.36%.

@@              Coverage Diff              @@
##             develop     #300      +/-   ##
=============================================
- Coverage      85.65%   85.61%   -0.04%     
- Complexity       644      661      +17     
=============================================
  Files             91       97       +6     
  Lines           2028     2078      +50     
=============================================
+ Hits            1737     1779      +42     
- Misses           291      299       +8     

Impacted Files	Coverage Δ	Complexity Δ
...ridge/Symfony/HttpFoundation/ResponseProcessor.php	75.00% <ø> (-14.48%)	2.00 <0.00> (-3.00)
...Foundation/SwooleRequestResponseContextManager.php	42.85% <42.85%> (ø)	3.00 <3.00> (?)
...tion/CompilerPass/StreamedResponseListenerPass.php	75.00% <75.00%> (ø)	2.00 <2.00> (?)
...y/HttpFoundation/NoOpStreamedResponseProcessor.php	83.33% <83.33%> (ø)	3.00 <3.00> (?)
...ymfony/HttpFoundation/StreamedResponseListener.php	83.33% <83.33%> (ø)	5.00 <5.00> (?)
src/Bridge/Symfony/Bundle/SwooleBundle.php	100.00% <100.00%> (ø)	1.00 <0.00> (ø)
...tpFoundation/ResponseHeadersAndStatusProcessor.php	100.00% <100.00%> (ø)	4.00 <4.00> (?)
...mfony/HttpFoundation/StreamedResponseProcessor.php	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...ge/Symfony/HttpKernel/HttpKernelRequestHandler.php	100.00% <100.00%> (ø)	4.00 <1.00> (ø)
... and 5 more

[k911]

Hi @supersmile2009, thanks for the changes. This weekend I'll try to add some missing tests for this & finally cut a release
(Don't worry about this code style issue. Probably I need to bump php-cs-fixer)

[supersmile2009]

@k911 I've fixed the failing tests and added a test for the new response handlers.
However some of the code style pipelines will keep failing and it's not related to the changes proposed in this PR.
Only latest-code-style or lowest-code-style pipeline would work, but not both at the same time. The problem is the version of php-cs-fixer installed by composer. @Symfony:risky rule set was changed recently (see PHP-CS-Fixer/PHP-CS-Fixer@3161ebf#diff-f3e1fe2f863ec817efb493212b57b2a2c1ecdab8741d83ceb0519dbaa7fad7abL50)
So if you run the old version of CS fixer, you have one config applied. If you run the latest version, you have a different config applied.

To mitigate that I'd recommend moving php-cs-fixer away from composer dependencies. CI should always install explicitly when needed (i. e. in code style job) and use the latest version (e. g. download it with curl). It will not only eliminate the issue of different rule sets, but also make sure nothing slips out unchecked/unfixed due to the bugs in older php-cs-fixer versions. Also it doesn't make much sense to run the php-cs-fixer with different dependency versions. In fact it can run with no dependencies installed at all, making the pipeline even faster.

[codeclimate]

Code Climate has analyzed commit 6e495e2 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 88.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 85.6% (0.0% change).

View more on Code Climate.

[k911]

@supersmile2009 Thanks for your update. As I commented before I agree that cs fixer issue is definitely out of scope for this PR. Although, I cannot agree with other points listed due to various reasons:

• cache (faster docker builds, a little but..)
• dev tools are also required to be up-to-date, because as you can see different version of tool = different results, and we want to keep consistency between several php versions, swoole versions and package versions, locking tools is the easiest way to achive that
• running code style tools on CI on different pipeline versions makes sure that future contributors can use any of the supported php version/latest/lowest supported dependencies to develop new code

IMO there aren't any benefits in not running them on multiple environments except wrong impression of having faster CI process, which then could cause issues that would have to be spotted manually. Also, if a tool produces different results across all supported versions it clearly means that we support incompatible version ranges and they have to be adjusted.

[Rastusik]

hi @k911 and @supersmile2009 :) I just found out that this feature just broke the development workload in our company.
To be more clear - we are integrating swoole bundle into our apps and until this feature, we were used to develop our app with swoole bundle enabled (for development purposes) but we still were able to serve our requests using php-fpm (our apps are older, so we're integrating swoole step by step and it takes some time to adapt the internals of the app). Coincindently the last app we are integrating is using StreamedResponses. With the current StreamedResponseListener, the inner Symfony http listeners are coupled to swoole, which breaks our php-fpm workloads.

What woud you say about an alternative approach to this problem, decoupled from swoole? it's quite similar to the firt approach of this PR - just with the change, that I would propose to set a callback attribute to the Symfony Request (on swoole to symfony request conversion). The callback would contain the code from the current StreamedResponseProcessor and the listener would use the callback from the request, if there was one, or it would run the code from the original symfony StreamedResponseListener if there would be no callback.

What would you say about this? I can implement it, but at first I'd like to know if the design is ok with you

[k911]

Hi @Rastusik, sure please propose PR from what I understood it's seems fine. Also, due to the fact that I treat this project more like a hobby and you're using it on production I must suggest that you should fork it internally in case you need to fix issues immediately and then you could backport features also to your codebase (and of course fixes here are most welcome)

P.S. With swoole I spend endless amount of hours trying to understood what is happening and why it isn't working especially when it comes to coroutines and testing (e.g.: https://app.circleci.com/pipelines/github/k911/swoole-bundle/1243/workflows/e5b1aecf-9ca6-4441-84c5-97a7234d9e72/jobs/14974 works ok, until it does not..). So I think you should reconsider using it on production, I found it working quite stable only for simple stateless (JSON) API applications (without ORM) or simple web servers. So good luck! :)

[Rastusik]

hi @k911 ok, I'll make a PR

regarding the forks, I would do it if there would be issues happening more frequently, but they aren't, and we always were able to workaround them when there were some, so I don't see a problem there

regarding production deployment - our app based on this bundle is serving cca 30 000 concurrent users daily for a year now and it was never unstable. It's a stateless app, but it uses Doctrine. So I'm not really afraid. Maybe what is hard to do is writing the tests, but on the app side it works well (we're still not using coroutines, but we plan to and that will be the time, when I can look at it too and help to improve the tests)