Fix #78413: php-fpm request_terminate_timeout does not take effect af…

[turchanov]

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

[rs-orlov]

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

[nikic]

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

[nikic]

This will make the timeout also apply in the FINISHED state. Is that right?

Additionally this also controls the slow log behavior...

[turchanov]

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.

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

[nikic]

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]

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]

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]

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

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.

[nikic]

Looks good to me. @bukka Anything to add?

[bukka]

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]

@turchanov can this be rebased for the requested branch please

[nikic]

Merged as e546d72 into PHP 7.3 and upwards.