Skip to content

Implement GH-12385: flush headers without body when calling flush() #12785

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Implement GH-12385: flush headers without body when calling flush() #12785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions sapi/cgi/cgi_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,11 @@ static void sapi_fcgi_flush(void *server_context)
{
fcgi_request *request = (fcgi_request*) server_context;

if (
!parent &&
request && !fcgi_flush(request, 0)) {

php_handle_aborted_connection();
if (!parent && request) {
sapi_send_headers();
if (!fcgi_flush(request, 0)) {
php_handle_aborted_connection();
}
}
}

Expand Down
7 changes: 5 additions & 2 deletions sapi/fpm/fpm/fpm_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,11 @@ static void sapi_cgibin_flush(void *server_context) /* {{{ */
/* fpm has started, let use fcgi instead of stdout */
if (fpm_is_running) {
fcgi_request *request = (fcgi_request*) server_context;
if (!parent && request && !fcgi_flush(request, 0)) {
php_handle_aborted_connection();
if (!parent && request) {
sapi_send_headers();
if (!fcgi_flush(request, 0)) {
php_handle_aborted_connection();
}
}
return;
}
Expand Down
46 changes: 46 additions & 0 deletions sapi/fpm/tests/gh12385.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
--TEST--
GH-12385 (flush with fastcgi does not force headers to be sent)
Copy link
Contributor

Choose a reason for hiding this comment

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

I confirmed in my testing that two FCGI packets will be sent: one for the header and one for the content, which is what is requested.

The feature request is about sending headers when flush() is called. I am not sure exactly what you meant by the cited sentence, but if the response buffer/body is empty, no (empty) packet for data should be sent.

If we have only few bytes to flush then nginx waits for more bytes to send in the TCP packet.

Am I understanding you right, headers are always sent with flush() (even with empty body) but small body is not sent even if flush() is called?

related discussion https://serverfault.com/questions/488767/how-do-i-enable-php-s-flush-with-nginxphp-fpm

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure exactly what you meant by the cited sentence, but if the response buffer/body is empty, no (empty) packet for data should be sent.

It means that an FCGI packet is sent for the header and body to the web server; whereas previously it would only send one. This implements the behaviour you want.

Am I understanding you right, headers are always sent with flush() (even with empty body) but small body is not sent even if flush() is called?

No. FPM will send it to the web server separately, but it all depends on what your web server does with it then, this is outside of the hands of PHP. The discussion link indeed summarizes it well.
Basically if you only send little data (be it headers or body content, doesn't matter) to nginx then it will wait for more data in the default configuration before sending data to the browser. That's what I wanted to highlight: that the usability highly depends of the behaviour of the web server.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah nginx buffers responses by default. You would need to disable buffering (fastcgi_buffering off) to potentially see impact of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on https://stackoverflow.com/questions/20018803/flush-output-buffer-in-apache-nginx-setup#23171223 and https://stackoverflow.com/questions/4870697/php-flush-that-works-even-in-nginx it seems a header from php can be sent to inform the nginx to not cache the response. FYI only.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GH-12385 (flush with fastcgi does not force headers to be sent)
FPM: GH-12385 - flush with fastcgi does not force headers to be sent

that's the format that I use for other FPM tests (mostly)

--SKIPIF--
<?php
include "skipif.inc";
?>
--FILE--
<?php

require_once "tester.inc";

$cfg = <<<EOT
[global]
error_log = {{FILE:LOG}}
[unconfined]
listen = {{ADDR}}
pm = static
pm.max_children = 1
EOT;

$code = <<<PHP
<?php
header("X-Test: 12345");
flush();
var_dump(headers_sent());
PHP;

$tester = new FPM\Tester($cfg, $code);
$tester->start();
$tester->expectLogStartNotices();
$response = $tester->request();
$response->expectHeader("X-Test", "12345");
$response->expectBody("bool(true)");
Comment on lines +24 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
flush();
var_dump(headers_sent());
PHP;
$tester = new FPM\Tester($cfg, $code);
$tester->start();
$tester->expectLogStartNotices();
$response = $tester->request();
$response->expectHeader("X-Test", "12345");
$response->expectBody("bool(true)");
$headersSentBeforeFlush = headers_sent();
flush();
$headersSentAfterFlush = headers_sent();
var_dump($headersSentBeforeFlush);
var_dump($headersSentAfterFlush);
PHP;
$tester = new FPM\Tester($cfg, $code);
$tester->start();
$tester->expectLogStartNotices();
$response = $tester->request();
$response->expectHeader("X-Test", "12345");
$response->expectBody("bool(false)\nbool(true)");

Copy link
Member

Choose a reason for hiding this comment

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

I don't this this is necessary. We don't need to test what's given which is that headers are not sent at the beginning of request.

$tester->terminate();
$tester->expectLogTerminatingNotices();
$tester->close();

?>
Done
--EXPECT--
Done
--CLEAN--
<?php
require_once "tester.inc";
FPM\Tester::clean();
?>