Skip to content

Flush backend output buffer after closing. #46931

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

Merged
merged 1 commit into from
Jul 16, 2022

Conversation

bradjones1
Copy link
Contributor

@bradjones1 bradjones1 commented Jul 13, 2022

Q A
Branch? 4.4
Bug fix? no yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

Currently, Response::send() does some backend/server specific shutdown, with the default case being closing userland output buffers. PHP's output buffering may or may not be On by default.

I recently discovered in the course of debugging my Drupal 9 project that closing output buffers may not be enough to flush the response's content to the client. In my case, output buffering wasn't enabled at all, making Response::closeOutputBuffers() a no-op. In either case, it appears necessary to also call flush(). This is important if the calling script/framework wishes to perform additional work after sending the request but before shutting down.

According to the PHP docs for flush(), this seems to be necessary in addition to closing all active output buffers (if used.)

Flushes the system write buffers of PHP and whatever backend PHP is using (CGI, a web server, etc). This attempts to push current output all the way to the browser with a few caveats.

flush() may not be able to override the buffering scheme of your web server and it has no effect on any client-side buffering in the browser. It also doesn't affect PHP's userspace output buffering mechanism. This means ob_flush() should be called before flush() to flush the output buffers if they are in use.

If it is Symfony's intent that the "final" flush() call be done by the implementing code/extending framework, it would be good to explicitly note that.

@carsonbot carsonbot added this to the 6.2 milestone Jul 13, 2022
@bradjones1 bradjones1 changed the title Flush backend output buffer after closing. [HttpFoundation] Flush backend output buffer after closing. Jul 13, 2022
@bradjones1
Copy link
Contributor Author

The PHP 8.2 test failure seems unrelated...

The Appveyor test seems to target Windows (?) and this is suspicious:

Symfony\Component\VarDumper\Tests\Server\ConnectionTest::testDump
Symfony\Component\Process\Exception\ProcessTimedOutException: The process "c:\php\php.exe" exceeded the timeout of 9 seconds.

I'll postpone digging too deeply into that until I get feedback on the general approach, though. If this is something Symfony will concern itself with, then we can address that.

@carsonbot
Copy link

Hey!

I think @abdounikarim has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@fabpot
Copy link
Member

fabpot commented Jul 15, 2022

It looks like we should consider this as a bug, so this should probably be rebased on 4.4?

@bradjones1
Copy link
Contributor Author

Thanks for the review, @fabpot.

It looks like we should consider this as a bug, so this should probably be rebased on 4.4?

I wasn't sure if this was a purposeful omission or not (I erred on the side of yes, it was) so I can recategorize and retarget as a bug.

@bradjones1 bradjones1 changed the base branch from 6.2 to 4.4 July 15, 2022 15:41
@carsonbot carsonbot changed the title [HttpFoundation] Flush backend output buffer after closing. Flush backend output buffer after closing. Jul 16, 2022
@fabpot fabpot modified the milestones: 6.2, 4.4 Jul 16, 2022
@fabpot
Copy link
Member

fabpot commented Jul 16, 2022

Thank you @bradjones1.

nicolas-grekas added a commit that referenced this pull request Aug 30, 2022
…utputBuffers (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] move flushing outside of Response::closeOutputBuffers

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Fix change introduced in #46931

Commits
-------

118acea [HttpFoundation] move flushing outside of Response::closeOutputBuffers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants