Skip to content

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

Closed
Closed
flush with fastcgi does not force headers to be sent#12385
Assignees
bukka
Labels
FeatureSAPI: cgiSAPI: fpm

Description

@mvorisek
mvorisek
opened on 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

Activity

  1. mvorisek
    added
    Bug
    Status: Needs Triage
    on Oct 8, 2023

  2. ndossche commented on Oct 8, 2023

    @ndossche
    ndossche
    on Oct 8, 2023
    Member

    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.

  3. mvorisek
    changed the title [-]`flush` with fastcgi does not force headers to be send[/-] [+]`flush` with fastcgi does not force headers to be sent[/+] on Oct 8, 2023
  4. ndossche
    added
    SAPI: fpm
    on Oct 8, 2023
  5. ndossche
    assigned
    bukka
    on Oct 8, 2023

  6. mvorisek commented on Oct 28, 2023

    @mvorisek
    mvorisek
    on Oct 28, 2023
    ContributorAuthor

    Is the issue present in FPM SAPI only?

  7. soltpain commented on Nov 1, 2023

    @soltpain
    soltpain
    on Nov 1, 2023

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

  8. bukka
    added
    SAPI: cgi
    and removed
    Status: Needs Triage
    on Nov 3, 2023

  9. bukka commented on Nov 9, 2023

    @bukka
    bukka
    on Nov 9, 2023
    Member

    @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.

  10. bukka
    removed
    SAPI: fpm
    on Nov 9, 2023

  11. bukka commented on Nov 9, 2023

    @bukka
    bukka
    on Nov 9, 2023
    Member

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

  12. bukka commented on Nov 9, 2023

    @bukka
    bukka
    on Nov 9, 2023
    Member

    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.

  13. bukka
    removed
    Bug
    on Nov 9, 2023

  14. 4 remaining items

  15. bukka
    reopened this on Nov 9, 2023

  16. bukka commented on Nov 9, 2023

    @bukka
    bukka
    on Nov 9, 2023
    Member

    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.

  17. ndossche commented on Nov 9, 2023

    @ndossche
    ndossche
    on Nov 9, 2023
    Member

    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.

  18. bukka commented on Nov 9, 2023

    @bukka
    bukka
    on Nov 9, 2023
    Member

    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... :)

  19. mvorisek commented on Nov 9, 2023

    @mvorisek
    mvorisek
    on Nov 9, 2023
    ContributorAuthor

    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.

  20. soltpain commented on Nov 10, 2023

    @soltpain
    soltpain
    on Nov 10, 2023

    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

  21. bukka commented on Nov 10, 2023

    @bukka
    bukka
    on Nov 10, 2023
    Member

    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.

  22. bukka commented on Nov 10, 2023

    @bukka
    bukka
    on Nov 10, 2023
    Member

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

  23. ndossche
    added a commit that references this issue on Nov 25, 2023

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

    a1facc9
  24. ndossche
    linked a pull request that will close this issueImplement GH-12385: flush headers without body when calling flush() #12785on Nov 25, 2023
  25. ndossche
    added a commit that references this issue on Dec 1, 2023

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

    6edbbc1

  26. ndossche commented on Dec 1, 2023

    @ndossche
    ndossche
    on Dec 1, 2023
    Member

    Implemented via 6edbbc1

  27. ndossche
    closed this as completedon Dec 1, 2023

  28. mvorisek commented on Dec 1, 2023

    @mvorisek
    mvorisek
    on Dec 1, 2023
    ContributorAuthor

    Thank you ❤️

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

Metadata

Metadata

Assignees

Labels

FeatureSAPI: cgiSAPI: fpm

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions