Skip to content

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

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

Closed
mvorisek opened this issue Oct 8, 2023 · 16 comments
Closed

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

mvorisek opened this issue Oct 8, 2023 · 16 comments

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Oct 8, 2023

Description

The following code:

<?php

var_dump(headers_sent());

while (ob_get_level() !== 0) {
    ob_end_flush();
}
flush();

var_dump(headers_sent());
var_dump(ob_get_level());

Resulted in this output:

bool(false)
bool(false)
int(0)

But I expected this output instead:

bool(false)
bool(true)
int(0)

This issue is reproducible with fastcgi on Windows together with nginx.

When I output/echo any data, the headers are always sent (expected).

When I use the php internal webserver instead, the flush is enough to force the headers to be send without any data/body (expected).

I belive this is a bug, as sending headers early must be supported. Some apps might need non trivial amount of time until first byte of the response is ready, also, some apps might not output any body at all, and the headers are not sent until the script ends.

PHP Version

8.1

Operating System

Windows 10

@nielsdos
Copy link
Member

nielsdos commented Oct 8, 2023

I can reproduce the behaviour on Linux. Based on my limited SAPI knowledge this patch might fix it:

diff --git a/sapi/fpm/fpm/fpm_main.c b/sapi/fpm/fpm/fpm_main.c
index b3ae2f69cc..12709ae063 100644
--- a/sapi/fpm/fpm/fpm_main.c
+++ b/sapi/fpm/fpm/fpm_main.c
@@ -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;
 	}

But I don't know the ins and outs of fpm, and not sure what other consequences this might have or if this is even desired. So I'll defer.

@mvorisek mvorisek changed the title flush with fastcgi does not force headers to be send flush with fastcgi does not force headers to be sent Oct 8, 2023
@mvorisek
Copy link
Contributor Author

Is the issue present in FPM SAPI only?

@soltpain
Copy link

soltpain commented Nov 1, 2023

Came here as also having an issue with sending headers without content

@bukka
Copy link
Member

bukka commented Nov 9, 2023

@nielsdos I have been just testing this with FPM and getting following output for the code above:

bool(false)
bool(true)
int(0)

Obviously the flush does not matter here because it sets that headers are send after the var_dump. I'm not sure I understand the test though so if there's something missing, please let me know and I will check.

Otherwise I also tested and debugged flush without sending output like below:

<?php
header('x-test1: yes');
flush();
sleep(1);
header('x-test2: yes');
var_dump(headers_sent());

So in this case I can that see that headers are not actually sent on flush because there is no content so only FCGI_BEGIN_REQUEST is sent. The FCGI_STDOUT followed FCGI_END_REQUEST is sent after the sleep (at the end of request.

Just to check that flush work I also tested this:

<?php
header('x-test1: yes');
echo 'data';
flush();
sleep(1);
echo 'more data';

So this send correctly two FCGI_STDOUT (first with headers and 'data' and second with just 'more data'). If there's no flush, then only one FCGI_STDOUT is sent. It works as expected though.

I don't see any issue in FPM. If you can see it, please let me know how to recreate it.

@bukka bukka removed the SAPI: fpm label Nov 9, 2023
@bukka
Copy link
Member

bukka commented Nov 9, 2023

Just for the reference here is my FPM setup that I used for testing: bukka/php-util@6203a71

@bukka
Copy link
Member

bukka commented Nov 9, 2023

Ah ok I see what's being requested now. Basically you want to send headers on flush if no content. That's somehow a breaking change so it could be considered as a feature request. It probably makes some sense but I don't see it as a bug.

@bukka
Copy link
Member

bukka commented Nov 9, 2023

It would be still useful if you could provide some info about the supplied example how it can return false on the first check as I have not been able to recreate it.

Btw the shortest code for this would be following:

<?php
flush();
var_dump(headers_sent());

@bukka bukka closed this as completed Nov 9, 2023
@bukka bukka reopened this Nov 9, 2023
@bukka
Copy link
Member

bukka commented Nov 9, 2023

Sorry about accidental closing...

And just to clarify, the change would be fine for master. The diff from @nielsdos seems like a right solution from a quick look. Should be done in cgi_main.c sapi_fcgi_flush as well and needs some test - only fpm test should be enough.

@nielsdos
Copy link
Member

nielsdos commented Nov 9, 2023

Agreed with your labeling as feature.

It would be still useful if you could provide some info about the supplied example how it can return false on the first check as I have not been able to recreate it.

I remember having reproduced it, but that I also had to modify the test. I don't seem to have the test file I used anymore though. I can look again in the weekend.

Should be done in cgi_main.c sapi_fcgi_flush as well and needs some test - only fpm test should be enough.

I can look into that in the weekend if you are not already doing so.

@bukka
Copy link
Member

bukka commented Nov 9, 2023

I can look into that in the weekend if you are not already doing so.

Feel free to take a look. I'm already looking to something else so I would just add this to my big todo list... :)

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 9, 2023

Ah ok I see what's being requested now. Basically you want to send headers on flush if no content. That's somehow a breaking change so it could be considered as a feature request. It probably makes some sense but I don't see it as a bug.

I am the original reporter. Yes, this is what is requested here - be able to dispatch headers /wo any body content.

@soltpain
Copy link

If you want an additional use case for this issue:

header('Content-Disposition:attachment;filename="report.txt"');
flush();
sleep(10);
echo 'report contents';

On Apache, it shows file dialog and then waits
On nginx (fpm), it waits, then shows file dialog

@bukka
Copy link
Member

bukka commented Nov 10, 2023

Once there's a PR for this, we will need to test this with various web servers to see how it works without body. I think it should be fine as headers are part of stdout but better to check.

@bukka
Copy link
Member

bukka commented Nov 10, 2023

But not sure if web server will flush it as well though so might be potentially not that useful. We will see.

@nielsdos
Copy link
Member

nielsdos commented Dec 1, 2023

Implemented via 6edbbc1

@nielsdos nielsdos closed this as completed Dec 1, 2023
@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 1, 2023

Thank you ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants