-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix #78413: php-fpm request_terminate_timeout does not take effect af… #4637
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
Conversation
…ter fastcgi_finish_request To retain legacy behavior I decided to add an option to control request termination logic. If request_terminate_strict is set then request will be tracked for time limits even after fastcgi_finish_request was called. This patch depends on the fix provided in BUG 78469 (otherwise php-fpm workers listening on named pipes on Windows will be erroneously terminated) (PR php#4636)
Is there some better name than |
sapi/fpm/fpm/fpm_request.c
Outdated
@@ -244,7 +244,7 @@ void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, | |||
} | |||
#endif | |||
|
|||
if (proc.request_stage > FPM_REQUEST_ACCEPTING && proc.request_stage < FPM_REQUEST_END) { | |||
if (proc.request_stage > FPM_REQUEST_ACCEPTING && ((proc.request_stage < FPM_REQUEST_END) || strict)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make the timeout also apply in the FINISHED
state. Is that right?
Additionally this also controls the slow log behavior...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make the timeout also apply in the FINISHED state. Is that right?
Yes, exactly. That was the intention. proc.request_stage
will be set to FINISHED either:
- (usually) in a request processing loop after
php_execute_script
finished executing current script, a call tofpm_request_end
is made which in turn sets FPM_REQUEST_FINISHED - (prematurely) by
fastcgi_finish_request
if FastCGI keep-alive is not active (as far as I know none of major webservers implement FastCGI keep-alive) via callchain:fastcgi_finish_request
->fcgi_close
->if (... !req->keep ... )
->req->hook.on_close
=fpm_request_finished
-> FPM_REQUEST_FINISHED.
And because requests in FPM_REQUEST_FINISHED are no longer tracked for timeout, this creates an opportunity for PHP code to seize control of worker process for arbitrary amount of time.
In addition to fastcgi_finish_request
, there is another way to exploit this bug by using shutdown functions (via register_shutdown_function
) since shutdown handlers are called in php_request_shutdown
which is called when request stage is already FINISHED.
After a careful review I came to conclusion that I might need to add a check for proc.request_stage <= FPM_REQUEST_FINISHED
just in case if some newer states will be added in future... What do you think? But from the other hand it will create a possibility to evade timeout timits in future ...
Additionally this also controls the slow log behavior...
Slow log explicitly checks for proc.request_stage == FPM_REQUEST_EXECUTING
so that even if we broaden set of request stages in parent if
it will be ok.
php-src/sapi/fpm/fpm/fpm_request.c
Lines 254 to 257 in 1ac6b02
if (child->slow_logged.tv_sec == 0 && slowlog_timeout && | |
proc.request_stage == FPM_REQUEST_EXECUTING && tv.tv_sec >= slowlog_timeout) { | |
str_purify_filename(purified_script_filename, proc.script_filename, sizeof(proc.script_filename)); |
Just wondering, rather than changing the behavior of request_terminate_timeout, might it make sense to have a second, separate timeout for the case of finished requests? I feel like there might be a use-case for having both those timeouts at different levels. |
The naming seems a bit strange to me as well and maybe another limit would make sense. The word |
After giving it some thought I think
Personally, I don't think we need another time limit to kill php-fpm workers. Because, in my opinion, |
Agree with @turchanov here. From perspective of worker pool control it doesn't matter when response has been sent. You just want to be sure that processing of any request can occupy worker no longer than some timeout being it synchronous (i.e. response blocking) or not.
|
Okay, let's stick with the single timeout for now and just improve the name of the option. No strong opinion on which one to use there. Both the |
comprehensible name 'request_terminate_timeout_track_finished' for the ini option.
Looks good to me. @bukka Anything to add? |
LGTM. Just don't think this should go to the 7.2 as it's a new feature... It's self contained but still a new configuration so maybe 7.3+ so it's only in the latest stable... |
@turchanov can this be rebased for the requested branch please |
Merged as e546d72 into PHP 7.3 and upwards. |
…ter fastcgi_finish_request
To retain legacy behavior I decided to add an option to control request
termination logic. If request_terminate_strict is set then request will
be tracked for time limits even after fastcgi_finish_request was called.
This patch depends on the fix provided in BUG 78469 (otherwise php-fpm
workers listening on named pipes on Windows will be erroneously terminated)
(PR #4636)