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.

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

Jump to bottom

Closed

nielsdos wants to merge 1 commit into php:master from nielsdos:flush-headers

Closed

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

nielsdos wants to merge 1 commit into php:master from nielsdos:flush-headers

Conversation

nielsdos

Copy link

Member

@nielsdos nielsdos commented Nov 25, 2023 •

edited

Loading

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.

I did notice however that on my nginx setup only one time it sends HTTP content to my browser.
However, that seems already to be the case with the flush functionality with normal body content. If we have only few bytes to flush then nginx waits for more bytes to send in the TCP packet. When creating a very large header is do see two times nginx sending HTTP data to my browser. I guess this is configurable in some way but just wanted to point out that this might have limited usefulness depending on the configuration.

@nielsdos


 Implement phpGH-12385 : flush headers without body when calling flush()

a1facc9

@nielsdos nielsdos requested a review from bukka as a code owner

November 25, 2023 23:15

@github-actions github-actions bot added SAPI: fpm SAPI: cgi labels

Nov 25, 2023

@nielsdos nielsdos linked an issue

Nov 25, 2023

that may be closed by this pull request

flush with fastcgi does not force headers to be sent #12385

Closed

mvorisek

mvorisek reviewed

Nov 26, 2023

View reviewed changes

sapi/fpm/tests/gh12385.phpt

Comment on lines +24 to +33

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)");

Copy link

Contributor

@mvorisek mvorisek Nov 26, 2023

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

@bukka bukka Dec 1, 2023

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.

mvorisek

mvorisek reviewed

Nov 26, 2023

View reviewed changes

sapi/fpm/tests/gh12385.phpt

@@ -0,0 +1,46 @@

--TEST--

GH-12385 (flush with fastcgi does not force headers to be sent)

Copy link

Contributor

@mvorisek mvorisek Nov 26, 2023

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?

Copy link

Member Author

@nielsdos nielsdos Nov 26, 2023

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.