implemented fastcgi_finish_request #113

dng-dev · 2016-10-11T19:25:35Z

Implemented fastcgi_finish_request() to send response to the client as early as possible.
This function flushes all response data to the client and finishes the request. This allows for time consuming tasks to be performed without leaving the connection to the client open.

Flyingmana

the php documentation has a comment about blocking trough session, but I dont think sessions are really blocking. (actually they even have problems with multiple parallel requests writing into the session)
But that problem would still exist without this addition

colinmollenhour · 2016-10-11T23:46:28Z

app/code/core/Mage/Core/Controller/Varien/Front.php

+        // to the client open.
+        if(php_sapi_name() == 'fpm-fcgi' && true === function_exists('fastcgi_finish_request')){
+            fastcgi_finish_request();
+        }
        Varien_Profiler::stop('mage::app::dispatch::send_response');
        Mage::dispatchEvent('controller_front_send_response_after', array('front'=>$this));


Maybe this should go after line 186 in case for some reason someone wants to inject some additional content like an html comment with cache debug info or similar?

I agree with @colinmollenhour about moving the location. I also don't see the value is the true === bit, it just makes it harder to read.

colinmollenhour · 2016-10-11T23:51:14Z

@Flyingmana Yes, session_write_close() can be used to close the session. I've been doing it this way for quite some time in index.php after Mage::run(...):

// Flush response before doing post-request processing
flush();
if (isset($_SESSION)) session_write_close();
if (function_exists('fastcgi_finish_request')) fastcgi_finish_request();

Wherever it goes just want to make sure it is after any events where a user might want to do some session modifications still.

dng-dev · 2016-10-12T14:23:01Z

I am not sure if this is right to echo stuff later. We use it in Production on this position for around 5/6 month with varnish and Ajax without any issues. Debug Stuff like Appserver Id we put as header.

Flyingmana · 2016-10-12T22:02:24Z

Iam for fastcgi_finish_request before the event, because this event should not output any html anymore, as the closing html tag was already send, also I think there is another event before to attach html.

I also would not close the session there, as the events listening afterwards probably need to read from it. And the point I was trying to do was, parallel session writing already can happen, and people should avoid writing to the session anyways.

colinmollenhour · 2016-10-12T22:17:24Z

because this event should not output any html anymore

Is this documented somewhere? I don't see why it can't be so just pointing out a potential BC breakage. Also, Magento is meant to be HMVC I believe so I think this needs to be moved out to a higher level.. For example:

https://github.com/colinmollenhour/Cm_Diehard/blob/master/code/Model/Backend/Local.php#L281

parallel session writing already can happen, and people should avoid writing to the session anyways.

Not sure I'm following you here.. What is "parallel session writing"?
The session_write_close() function happens anyway on shutdown, this method just triggers it sooner than later. So actually it would make sense to do it after fastcgi_finish_request(), but the advantage to calling it at all vs not calling it is that if you are using a backend which does locking then the lock will be released sooner. There is no reason to hold the lock on a session after no more assignments will be made to it.

// Flush response before doing post-request processing
flush();
if (function_exists('fastcgi_finish_request')) fastcgi_finish_request();
if (isset($_SESSION)) session_write_close();

colinmollenhour · 2016-10-12T22:27:51Z

I propose putting it after this line so that it is guaranteed to be BC:

https://github.com/OpenMage/magento-lts/blob/1.9.2.4/app/Mage.php#L685

Flyingmana · 2016-10-13T21:54:33Z

I looked a bit into the event, what it is used for. And we cant close the session before it. Also we have to expect that people make use of the session for such long processing cases we want to support here.

I will explain my point with the session_write a bit. Depending on the used backend and the implementation the Session can be a single "big" dataset. This is written on session_close. If the process still runs, and another one is already started, it has no access to the session changes of the long running process. And if the long running process runs longer then the 2. or even 3. process for this one session, it can happen, that it overwrites their session changes. I just wanted to mention this, I actually dont think we should add something for handling this, as the wished handling of this can depend a lot on the shop and the extensions doing such long running stuff. Some of them may require the session, others not.

regarding the fastcgi_finish_request() before or after the event. Is there even a later event which is commonly used for longer running processing?
Also, the event is already called "..._send_response_after" so it should be expected, the response is already sent. Any extension which now would output something, would do this knowing, the closing tag was already output, and would be willing to create broken html.
There may be extensions doing this, but they are probably edge cases and causing broken html.
So this BC break will very likely only break already broken stuff.

colinmollenhour · 2016-10-14T00:59:18Z

Please read again: If the user doesn't want to output more data after the response is sent, calling fastcgi_finish_request inside of the front dispatch() call stack is still bad because the HMVC pattern allows the "response" to be not necessarily flushed which is exactly what I do here in my cache extension:

https://github.com/colinmollenhour/Cm_Diehard/blob/master/code/Model/Backend/Local.php#L281

Maybe I'm the only one that ever did this with Magento, but still there is no harm in moving it up the call stack one frame..

LeeSaferite · 2016-10-16T16:46:38Z

app/code/core/Mage/Core/Controller/Varien/Front.php

+        // to the client open.
+        if(php_sapi_name() == 'fpm-fcgi' && true === function_exists('fastcgi_finish_request')){
+            fastcgi_finish_request();
+        }
        Varien_Profiler::stop('mage::app::dispatch::send_response');
        Mage::dispatchEvent('controller_front_send_response_after', array('front'=>$this));


I agree with @colinmollenhour about moving the location. I also don't see the value is the true === bit, it just makes it harder to read.

seansan · 2018-02-08T13:43:08Z

Pfoe. That was hard to read ;)
Is this still relevant? And does it solve a real bug/problem?

colinmollenhour · 2018-10-04T17:59:18Z

Yes, I think it is a good improvement, just belongs in a different location.

addison74 · 2021-02-23T14:17:25Z

There are situations when the client doesn't wait for server response or resets the connection (e.g. bots). In this case Apache/Nginx error logs will fill up with lines like this one:

[Fri Jan 09 16:50:00.093819 2015] [fcgid:warn] [pid 26063] (104)Connection reset by peer: [client <IPv4_ADDRESS>:9784] mod_fcgid: ap_pass_brigade failed in handle_request_ipc function

Many system administrators trying to find a solution start to adjust FCGI parameters in Apache or PHP configuration files but this doesn't solve the issue. Also it is not coming from a slow script in Magento as a few stated in Forums. If they will check the access logs they will understand only a few IP's generate this issue from time to time those IP's which are banned for SPAM or running bots.

As I understand this commit comes to solve an issue like this in Magento. If it works without generating any trouble indeed it is an improvement.

…mpleted rendering. Add core_app_run_after to allow events to occur after response is sent. Refs OpenMage#113

fballiano · 2022-04-20T11:01:32Z

Yes, I think it is a good improvement, just belongs in a different location.

what would the location be now? so that we could modify the PR and move it forward ;-)

addison74 · 2022-06-02T19:54:46Z

This PR needs to be rebased to OpenMage:1.9.4.x.

The base branch was changed.

addison74 · 2023-01-13T23:12:27Z

I would close this PR in favor of #1592. Let's see some 2 - 3 opinions too.

@colinmollenhour - what do you think about?

luigifab · 2023-03-04T09:36:08Z

I vote for 1592.

…vent (#1592) * Explicitly end the request and close the session after the app has completed rendering. Add core_app_run_after to allow events to occur after response is sent. Refs #113 * Update EVENTS.md --------- Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>

implemented fastcgi_finish_request

1d47fb2

Flyingmana previously approved these changes Oct 11, 2016

View reviewed changes

colinmollenhour reviewed Oct 11, 2016

View reviewed changes

LeeSaferite suggested changes Oct 16, 2016

View reviewed changes

tbaden added the code change requested For Tickets where changes in code are needed, but seem mostly fine label Jun 1, 2019

sreichel added Component: Core Relates to Mage_Core enhancement labels Jun 5, 2020

colinmollenhour mentioned this pull request May 3, 2021

Ended the request (and close the session) before core_app_run_after event #1592

Merged

3 tasks

fballiano changed the base branch from 1.9.2.4 to 1.9.4.x June 3, 2022 08:28

fballiano closed this in #1592 Apr 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented fastcgi_finish_request #113

implemented fastcgi_finish_request #113

dng-dev commented Oct 11, 2016

Flyingmana left a comment

colinmollenhour Oct 11, 2016

LeeSaferite Oct 16, 2016

colinmollenhour commented Oct 11, 2016

dng-dev commented Oct 12, 2016

Flyingmana commented Oct 12, 2016

colinmollenhour commented Oct 12, 2016

colinmollenhour commented Oct 12, 2016

Flyingmana commented Oct 13, 2016 •

edited

Loading

colinmollenhour commented Oct 14, 2016

LeeSaferite Oct 16, 2016

seansan commented Feb 8, 2018

colinmollenhour commented Oct 4, 2018

addison74 commented Feb 23, 2021 •

edited

Loading

fballiano commented Apr 20, 2022

addison74 commented Jun 2, 2022

addison74 commented Jan 13, 2023

luigifab commented Mar 4, 2023

implemented fastcgi_finish_request #113

implemented fastcgi_finish_request #113

Conversation

dng-dev commented Oct 11, 2016

Flyingmana left a comment

Choose a reason for hiding this comment

colinmollenhour Oct 11, 2016

Choose a reason for hiding this comment

LeeSaferite Oct 16, 2016

Choose a reason for hiding this comment

colinmollenhour commented Oct 11, 2016

dng-dev commented Oct 12, 2016

Flyingmana commented Oct 12, 2016

colinmollenhour commented Oct 12, 2016

colinmollenhour commented Oct 12, 2016

Flyingmana commented Oct 13, 2016 • edited Loading

colinmollenhour commented Oct 14, 2016

LeeSaferite Oct 16, 2016

Choose a reason for hiding this comment

seansan commented Feb 8, 2018

colinmollenhour commented Oct 4, 2018

addison74 commented Feb 23, 2021 • edited Loading

fballiano commented Apr 20, 2022

addison74 commented Jun 2, 2022

addison74 commented Jan 13, 2023

luigifab commented Mar 4, 2023

Flyingmana commented Oct 13, 2016 •

edited

Loading

addison74 commented Feb 23, 2021 •

edited

Loading