Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

turchanov
Copy link
Contributor

…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)

…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)
@cmb69 cmb69 added the Bug label Aug 28, 2019
@rs-orlov
Copy link

rs-orlov commented Sep 9, 2019

Hi.
Related fix (#4636) was merged pretty fast, so, is there something wrong with that one? @nikic

@nikic nikic requested a review from bukka September 9, 2019 15:55
@nikic
Copy link
Member

nikic commented Sep 9, 2019

Is there some better name than request_terminate_strict we could use? It's not really clear what this does without reading docs.

@@ -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)) {
Copy link
Member

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

Copy link
Contributor Author

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 to fpm_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.

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

@nikic
Copy link
Member

nikic commented Sep 9, 2019

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.

@bukka
Copy link
Member

bukka commented Sep 9, 2019

The naming seems a bit strange to me as well and maybe another limit would make sense. The word request would not really apply here as it's after the end of request. It's some kind post-request. Although that could suggest POST method to someone so maybe not the best name either...

@turchanov
Copy link
Contributor Author

After giving it some thought I think request_terminate_timeout_policy would have been a better option, with values "standard" (retaining current behaviour) and "strict" (that is apply timeout regardless of "fastcgi_finish_request" having been called or not)?
Or, maybe, request_terminate_timeout_track_finished = (false|true)?

... might it make sense to have a second, separate timeout for the case of finished requests?

Personally, I don't think we need another time limit to kill php-fpm workers. Because, in my opinion, request_terminate_timeout primary use is to protect from exhaustion of worker pool by too lengthy requests (be they in "interactive" state or in "post-processing" state).
But if you want I can add another timelimit option, but I cannot think of any decent name for it 😟. Maybe request_finished_state_terminate_timeout? But I am afraid that it may create a confusion for users of what timeout applies to what.

@rs-orlov
Copy link

... might it make sense to have a second, separate timeout for the case of finished requests?

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.

request_terminate_timeout_policy is a decent name, but options like only_interactive and include_post_processing might have been a bit more understandable

@nikic
Copy link
Member

nikic commented Sep 17, 2019

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 request_terminate_timeout_policy and request_terminate_timeout_track_finished variants sound fine to me.

comprehensible name 'request_terminate_timeout_track_finished'
for the ini option.
@nikic
Copy link
Member

nikic commented Sep 25, 2019

Looks good to me. @bukka Anything to add?

@bukka
Copy link
Member

bukka commented Sep 25, 2019

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

@krakjoe
Copy link
Member

krakjoe commented Sep 30, 2019

@turchanov can this be rebased for the requested branch please

@nikic nikic self-assigned this Sep 30, 2019
@nikic
Copy link
Member

nikic commented Sep 30, 2019

Merged as e546d72 into PHP 7.3 and upwards.

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

Successfully merging this pull request may close these issues.

6 participants