From 57e6699963482b50d73ac03f778524fbb0473380 Mon Sep 17 00:00:00 2001 From: Sergei Turchanov Date: Wed, 28 Aug 2019 14:37:52 +1000 Subject: [PATCH 1/2] Fix #78413: php-fpm request_terminate_timeout does not take effect after 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) --- sapi/fpm/fpm/fpm_conf.c | 2 ++ sapi/fpm/fpm/fpm_conf.h | 1 + sapi/fpm/fpm/fpm_process_ctl.c | 3 ++- sapi/fpm/fpm/fpm_request.c | 4 ++-- sapi/fpm/fpm/fpm_request.h | 2 +- sapi/fpm/www.conf.in | 6 ++++++ 6 files changed, 14 insertions(+), 4 deletions(-) diff --git a/sapi/fpm/fpm/fpm_conf.c b/sapi/fpm/fpm/fpm_conf.c index 42ee60b36cce5..16ef684e0b99a 100644 --- a/sapi/fpm/fpm/fpm_conf.c +++ b/sapi/fpm/fpm/fpm_conf.c @@ -148,6 +148,7 @@ static struct ini_value_parser_s ini_fpm_pool_options[] = { { "request_slowlog_timeout", &fpm_conf_set_time, WPO(request_slowlog_timeout) }, { "request_slowlog_trace_depth", &fpm_conf_set_integer, WPO(request_slowlog_trace_depth) }, { "request_terminate_timeout", &fpm_conf_set_time, WPO(request_terminate_timeout) }, + { "request_terminate_strict", &fpm_conf_set_boolean, WPO(request_terminate_strict) }, { "rlimit_files", &fpm_conf_set_integer, WPO(rlimit_files) }, { "rlimit_core", &fpm_conf_set_rlimit_core, WPO(rlimit_core) }, { "chroot", &fpm_conf_set_string, WPO(chroot) }, @@ -1664,6 +1665,7 @@ static void fpm_conf_dump() /* {{{ */ zlog(ZLOG_NOTICE, "\trequest_slowlog_timeout = %ds", wp->config->request_slowlog_timeout); zlog(ZLOG_NOTICE, "\trequest_slowlog_trace_depth = %d", wp->config->request_slowlog_trace_depth); zlog(ZLOG_NOTICE, "\trequest_terminate_timeout = %ds", wp->config->request_terminate_timeout); + zlog(ZLOG_NOTICE, "\trequest_terminate_strict = %s", BOOL2STR(wp->config->request_terminate_strict)); zlog(ZLOG_NOTICE, "\trlimit_files = %d", wp->config->rlimit_files); zlog(ZLOG_NOTICE, "\trlimit_core = %d", wp->config->rlimit_core); zlog(ZLOG_NOTICE, "\tchroot = %s", STR2STR(wp->config->chroot)); diff --git a/sapi/fpm/fpm/fpm_conf.h b/sapi/fpm/fpm/fpm_conf.h index 2f11953367f19..97ab0821aec47 100644 --- a/sapi/fpm/fpm/fpm_conf.h +++ b/sapi/fpm/fpm/fpm_conf.h @@ -80,6 +80,7 @@ struct fpm_worker_pool_config_s { int request_slowlog_timeout; int request_slowlog_trace_depth; int request_terminate_timeout; + int request_terminate_strict; int rlimit_files; int rlimit_core; char *chroot; diff --git a/sapi/fpm/fpm/fpm_process_ctl.c b/sapi/fpm/fpm/fpm_process_ctl.c index f99ec241c74eb..2613859764e00 100644 --- a/sapi/fpm/fpm/fpm_process_ctl.c +++ b/sapi/fpm/fpm/fpm_process_ctl.c @@ -293,13 +293,14 @@ static void fpm_pctl_check_request_timeout(struct timeval *now) /* {{{ */ struct fpm_worker_pool_s *wp; for (wp = fpm_worker_all_pools; wp; wp = wp->next) { + int strict = wp->config->request_terminate_strict; int terminate_timeout = wp->config->request_terminate_timeout; int slowlog_timeout = wp->config->request_slowlog_timeout; struct fpm_child_s *child; if (terminate_timeout || slowlog_timeout) { for (child = wp->children; child; child = child->next) { - fpm_request_check_timed_out(child, now, terminate_timeout, slowlog_timeout); + fpm_request_check_timed_out(child, now, terminate_timeout, slowlog_timeout, strict); } } } diff --git a/sapi/fpm/fpm/fpm_request.c b/sapi/fpm/fpm/fpm_request.c index a4ace85310425..4912b79bfd798 100644 --- a/sapi/fpm/fpm/fpm_request.c +++ b/sapi/fpm/fpm/fpm_request.c @@ -222,7 +222,7 @@ void fpm_request_finished() /* {{{ */ } /* }}} */ -void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, int terminate_timeout, int slowlog_timeout) /* {{{ */ +void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, int terminate_timeout, int slowlog_timeout, int strict) /* {{{ */ { struct fpm_scoreboard_proc_s proc, *proc_p; @@ -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)) { char purified_script_filename[sizeof(proc.script_filename)]; struct timeval tv; diff --git a/sapi/fpm/fpm/fpm_request.h b/sapi/fpm/fpm/fpm_request.h index 1328b5de4e350..363d8b1a0d80a 100644 --- a/sapi/fpm/fpm/fpm_request.h +++ b/sapi/fpm/fpm/fpm_request.h @@ -14,7 +14,7 @@ void fpm_request_finished(); /* request processed: cleaning current request * struct fpm_child_s; struct timeval; -void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *tv, int terminate_timeout, int slowlog_timeout); +void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *tv, int terminate_timeout, int slowlog_timeout, int strict); int fpm_request_is_idle(struct fpm_child_s *child); const char *fpm_request_get_stage_name(int stage); int fpm_request_last_activity(struct fpm_child_s *child, struct timeval *tv); diff --git a/sapi/fpm/www.conf.in b/sapi/fpm/www.conf.in index ccfdbd9e872fd..b624fd80532fb 100644 --- a/sapi/fpm/www.conf.in +++ b/sapi/fpm/www.conf.in @@ -339,6 +339,12 @@ pm.max_spare_servers = 3 ; Default Value: 0 ;request_terminate_timeout = 0 +; When application calls fastcgi_finish_request it will inhibit request +; termination after request_terminate_timeout time limit. This option will enable +; timeout limit to be applied unconditionally even after fastcgi_finish_request. +; To retain legacy behavior the default is 'no' +;request_terminate_strict = no + ; Set open file descriptor rlimit. ; Default Value: system defined value ;rlimit_files = 1024 From 088d9f012e22da8964c5d387f6ec14ef338bb616 Mon Sep 17 00:00:00 2001 From: Sergei Turchanov Date: Mon, 23 Sep 2019 21:42:03 +1000 Subject: [PATCH 2/2] Changes suggested by code reviewer (nikic). We have settled for a more comprehensible name 'request_terminate_timeout_track_finished' for the ini option. --- sapi/fpm/fpm/fpm_conf.c | 4 ++-- sapi/fpm/fpm/fpm_conf.h | 2 +- sapi/fpm/fpm/fpm_process_ctl.c | 4 ++-- sapi/fpm/fpm/fpm_request.c | 4 ++-- sapi/fpm/fpm/fpm_request.h | 2 +- sapi/fpm/www.conf.in | 12 +++++++----- 6 files changed, 15 insertions(+), 13 deletions(-) diff --git a/sapi/fpm/fpm/fpm_conf.c b/sapi/fpm/fpm/fpm_conf.c index 16ef684e0b99a..93e6a2ff9c298 100644 --- a/sapi/fpm/fpm/fpm_conf.c +++ b/sapi/fpm/fpm/fpm_conf.c @@ -148,7 +148,7 @@ static struct ini_value_parser_s ini_fpm_pool_options[] = { { "request_slowlog_timeout", &fpm_conf_set_time, WPO(request_slowlog_timeout) }, { "request_slowlog_trace_depth", &fpm_conf_set_integer, WPO(request_slowlog_trace_depth) }, { "request_terminate_timeout", &fpm_conf_set_time, WPO(request_terminate_timeout) }, - { "request_terminate_strict", &fpm_conf_set_boolean, WPO(request_terminate_strict) }, + { "request_terminate_timeout_track_finished", &fpm_conf_set_boolean, WPO(request_terminate_timeout_track_finished) }, { "rlimit_files", &fpm_conf_set_integer, WPO(rlimit_files) }, { "rlimit_core", &fpm_conf_set_rlimit_core, WPO(rlimit_core) }, { "chroot", &fpm_conf_set_string, WPO(chroot) }, @@ -1665,7 +1665,7 @@ static void fpm_conf_dump() /* {{{ */ zlog(ZLOG_NOTICE, "\trequest_slowlog_timeout = %ds", wp->config->request_slowlog_timeout); zlog(ZLOG_NOTICE, "\trequest_slowlog_trace_depth = %d", wp->config->request_slowlog_trace_depth); zlog(ZLOG_NOTICE, "\trequest_terminate_timeout = %ds", wp->config->request_terminate_timeout); - zlog(ZLOG_NOTICE, "\trequest_terminate_strict = %s", BOOL2STR(wp->config->request_terminate_strict)); + zlog(ZLOG_NOTICE, "\trequest_terminate_timeout_track_finished = %s", BOOL2STR(wp->config->request_terminate_timeout_track_finished)); zlog(ZLOG_NOTICE, "\trlimit_files = %d", wp->config->rlimit_files); zlog(ZLOG_NOTICE, "\trlimit_core = %d", wp->config->rlimit_core); zlog(ZLOG_NOTICE, "\tchroot = %s", STR2STR(wp->config->chroot)); diff --git a/sapi/fpm/fpm/fpm_conf.h b/sapi/fpm/fpm/fpm_conf.h index 97ab0821aec47..f8dd4fa85d38d 100644 --- a/sapi/fpm/fpm/fpm_conf.h +++ b/sapi/fpm/fpm/fpm_conf.h @@ -80,7 +80,7 @@ struct fpm_worker_pool_config_s { int request_slowlog_timeout; int request_slowlog_trace_depth; int request_terminate_timeout; - int request_terminate_strict; + int request_terminate_timeout_track_finished; int rlimit_files; int rlimit_core; char *chroot; diff --git a/sapi/fpm/fpm/fpm_process_ctl.c b/sapi/fpm/fpm/fpm_process_ctl.c index 2613859764e00..2e631f75fa96a 100644 --- a/sapi/fpm/fpm/fpm_process_ctl.c +++ b/sapi/fpm/fpm/fpm_process_ctl.c @@ -293,14 +293,14 @@ static void fpm_pctl_check_request_timeout(struct timeval *now) /* {{{ */ struct fpm_worker_pool_s *wp; for (wp = fpm_worker_all_pools; wp; wp = wp->next) { - int strict = wp->config->request_terminate_strict; + int track_finished = wp->config->request_terminate_timeout_track_finished; int terminate_timeout = wp->config->request_terminate_timeout; int slowlog_timeout = wp->config->request_slowlog_timeout; struct fpm_child_s *child; if (terminate_timeout || slowlog_timeout) { for (child = wp->children; child; child = child->next) { - fpm_request_check_timed_out(child, now, terminate_timeout, slowlog_timeout, strict); + fpm_request_check_timed_out(child, now, terminate_timeout, slowlog_timeout, track_finished); } } } diff --git a/sapi/fpm/fpm/fpm_request.c b/sapi/fpm/fpm/fpm_request.c index 4912b79bfd798..5740a196aadfd 100644 --- a/sapi/fpm/fpm/fpm_request.c +++ b/sapi/fpm/fpm/fpm_request.c @@ -222,7 +222,7 @@ void fpm_request_finished() /* {{{ */ } /* }}} */ -void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, int terminate_timeout, int slowlog_timeout, int strict) /* {{{ */ +void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *now, int terminate_timeout, int slowlog_timeout, int track_finished) /* {{{ */ { struct fpm_scoreboard_proc_s proc, *proc_p; @@ -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) || strict)) { + if (proc.request_stage > FPM_REQUEST_ACCEPTING && ((proc.request_stage < FPM_REQUEST_END) || track_finished)) { char purified_script_filename[sizeof(proc.script_filename)]; struct timeval tv; diff --git a/sapi/fpm/fpm/fpm_request.h b/sapi/fpm/fpm/fpm_request.h index 363d8b1a0d80a..f8e413d49052c 100644 --- a/sapi/fpm/fpm/fpm_request.h +++ b/sapi/fpm/fpm/fpm_request.h @@ -14,7 +14,7 @@ void fpm_request_finished(); /* request processed: cleaning current request * struct fpm_child_s; struct timeval; -void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *tv, int terminate_timeout, int slowlog_timeout, int strict); +void fpm_request_check_timed_out(struct fpm_child_s *child, struct timeval *tv, int terminate_timeout, int slowlog_timeout, int track_finished); int fpm_request_is_idle(struct fpm_child_s *child); const char *fpm_request_get_stage_name(int stage); int fpm_request_last_activity(struct fpm_child_s *child, struct timeval *tv); diff --git a/sapi/fpm/www.conf.in b/sapi/fpm/www.conf.in index b624fd80532fb..53b07797bca8f 100644 --- a/sapi/fpm/www.conf.in +++ b/sapi/fpm/www.conf.in @@ -339,11 +339,13 @@ pm.max_spare_servers = 3 ; Default Value: 0 ;request_terminate_timeout = 0 -; When application calls fastcgi_finish_request it will inhibit request -; termination after request_terminate_timeout time limit. This option will enable -; timeout limit to be applied unconditionally even after fastcgi_finish_request. -; To retain legacy behavior the default is 'no' -;request_terminate_strict = no +; The timeout set by 'request_terminate_timeout' ini option is not engaged after +; application calls 'fastcgi_finish_request' or when application has finished and +; shutdown functions are being called (registered via register_shutdown_function). +; This option will enable timeout limit to be applied unconditionally +; even in such cases. +; Default Value: no +;request_terminate_timeout_track_finished = no ; Set open file descriptor rlimit. ; Default Value: system defined value