Hello!
Looks like to me that the original patch does what it's supposed to do
(when combined with http://hg.nginx.org/nginx/rev/3069dd358ba2). Here is my
understanding:
Before this patch, an active connection could potentially delay shutdown
indefinitely due to the presence of connection related timer inside the
timer tree. The only reason why ngx_shutdown_timer_handler has to be
invoked would be to force those lingering connections to be closed and
therefore, free-up the timer tree and allow the shutdown to finish.
If all connections were closed before ngx_shutdown_timer_handler was fired
(your case), it is already safe to finish the shutdown and there is no
reason to invoke or wait for ngx_shutdown_timer_handler to fire anymore.
The cancelable field makes sure when this happens ngx_shutdown_event does
not prevent NGINX from being able to complete the shutdown.
Seems to me that delaying the shutdown until worker_shutdown_timeout when
there is already no active connection left makes little sense.
Thanks,
Datong
On Tue, Mar 7, 2017 at 11:52 PM, 洪志道 <hongzhi...@gmail.com> wrote:
> Hi!
>
> It's a good design.
> It seems the worker process is not killed until expired, the time is set
> by 'worker_shutdown_timeout'.
> But I think ngx_shutdown_timer_handler will never be called, because of
> the following deal.
>
> if (ngx_exiting) {
> if (ngx_event_no_timers_left() == NGX_OK) {
> ngx_log_error(NGX_LOG_NOTICE, cycle->log, 0, "exiting");
> ngx_worker_process_exit(cycle); // the worker process
> exits though there are cancelable timers such as ngx_shutdown_event.
> }
> }
>
> I'm not sure it's right?
>
> diff -r d45072375572 src/core/ngx_cycle.c
> --- a/src/core/ngx_cycle.c Tue Mar 07 18:51:17 2017 +0300
> +++ b/src/core/ngx_cycle.c Sun Mar 05 17:03:22 2017 +0800
> @@ -1348,7 +1348,6 @@
> ngx_shutdown_event.handler = ngx_shutdown_timer_handler;
> ngx_shutdown_event.data = cycle;
> ngx_shutdown_event.log = cycle->log;
> - ngx_shutdown_event.cancelable = 1;
>
> ngx_add_timer(&ngx_shutdown_event, ccf->shutdown_timeout);
> }
>
> And I think it's better that the shutting down process keep alive until '
> worker_shutdown_timeout'.
>
> Thanks.
> B.R.
>
> 2017-03-08 1:01 GMT+08:00 Maxim Dounin <mdou...@mdounin.ru>:
>
>> details: http://hg.nginx.org/nginx/rev/97c99bb43737
>> branches:
>> changeset: 6930:97c99bb43737
>> user: Maxim Dounin <mdou...@mdounin.ru>
>> date: Tue Mar 07 18:51:16 2017 +0300
>> description:
>> Introduced worker_shutdown_timeout.
>>
>> The directive configures a timeout to be used when gracefully shutting
>> down
>> worker processes. When the timer expires, nginx will try to close all
>> the connections currently open to facilitate shutdown.
>>
>> diffstat:
>>
>> src/core/nginx.c | 9 ++++++
>> src/core/ngx_cycle.c | 53 ++++++++++++++++++++++++++++++
>> ++++++++++
>> src/core/ngx_cycle.h | 2 +
>> src/os/unix/ngx_process_cycle.c | 1 +
>> src/os/win32/ngx_process_cycle.c | 1 +
>> 5 files changed, 66 insertions(+), 0 deletions(-)
>>
>> diffs (148 lines):
>>
>> diff --git a/src/core/nginx.c b/src/core/nginx.c
>> --- a/src/core/nginx.c
>> +++ b/src/core/nginx.c
>> @@ -124,6 +124,13 @@ static ngx_command_t ngx_core_commands[
>> offsetof(ngx_core_conf_t, rlimit_core),
>> NULL },
>>
>> + { ngx_string("worker_shutdown_timeout"),
>> + NGX_MAIN_CONF|NGX_DIRECT_CONF|NGX_CONF_TAKE1,
>> + ngx_conf_set_msec_slot,
>> + 0,
>> + offsetof(ngx_core_conf_t, shutdown_timeout),
>> + NULL },
>> +
>> { ngx_string("working_directory"),
>> NGX_MAIN_CONF|NGX_DIRECT_CONF|NGX_CONF_TAKE1,
>> ngx_conf_set_str_slot,
>> @@ -1014,6 +1021,7 @@ ngx_core_module_create_conf(ngx_cycle_t
>> ccf->daemon = NGX_CONF_UNSET;
>> ccf->master = NGX_CONF_UNSET;
>> ccf->timer_resolution = NGX_CONF_UNSET_MSEC;
>> + ccf->shutdown_timeout = NGX_CONF_UNSET_MSEC;
>>
>> ccf->worker_processes = NGX_CONF_UNSET;
>> ccf->debug_points = NGX_CONF_UNSET;
>> @@ -1042,6 +1050,7 @@ ngx_core_module_init_conf(ngx_cycle_t *c
>> ngx_conf_init_value(ccf->daemon, 1);
>> ngx_conf_init_value(ccf->master, 1);
>> ngx_conf_init_msec_value(ccf->timer_resolution, 0);
>> + ngx_conf_init_msec_value(ccf->shutdown_timeout, 0);
>>
>> ngx_conf_init_value(ccf->worker_processes, 1);
>> ngx_conf_init_value(ccf->debug_points, 0);
>> diff --git a/src/core/ngx_cycle.c b/src/core/ngx_cycle.c
>> --- a/src/core/ngx_cycle.c
>> +++ b/src/core/ngx_cycle.c
>> @@ -15,6 +15,7 @@ static ngx_int_t ngx_init_zone_pool(ngx_
>> ngx_shm_zone_t *shm_zone);
>> static ngx_int_t ngx_test_lockfile(u_char *file, ngx_log_t *log);
>> static void ngx_clean_old_cycles(ngx_event_t *ev);
>> +static void ngx_shutdown_timer_handler(ngx_event_t *ev);
>>
>>
>> volatile ngx_cycle_t *ngx_cycle;
>> @@ -22,6 +23,7 @@ ngx_array_t ngx_old_cycles;
>>
>> static ngx_pool_t *ngx_temp_pool;
>> static ngx_event_t ngx_cleaner_event;
>> +static ngx_event_t ngx_shutdown_event;
>>
>> ngx_uint_t ngx_test_config;
>> ngx_uint_t ngx_dump_config;
>> @@ -1333,3 +1335,54 @@ ngx_clean_old_cycles(ngx_event_t *ev)
>> ngx_old_cycles.nelts = 0;
>> }
>> }
>> +
>> +
>> +void
>> +ngx_set_shutdown_timer(ngx_cycle_t *cycle)
>> +{
>> + ngx_core_conf_t *ccf;
>> +
>> + ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx,
>> ngx_core_module);
>> +
>> + if (ccf->shutdown_timeout) {
>> + ngx_shutdown_event.handler = ngx_shutdown_timer_handler;
>> + ngx_shutdown_event.data = cycle;
>> + ngx_shutdown_event.log = cycle->log;
>> + ngx_shutdown_event.cancelable = 1;
>> +
>> + ngx_add_timer(&ngx_shutdown_event, ccf->shutdown_timeout);
>> + }
>> +}
>> +
>> +
>> +static void
>> +ngx_shutdown_timer_handler(ngx_event_t *ev)
>> +{
>> + ngx_uint_t i;
>> + ngx_cycle_t *cycle;
>> + ngx_connection_t *c;
>> +
>> + cycle = ev->data;
>> +
>> + c = cycle->connections;
>> +
>> + for (i = 0; i < cycle->connection_n; i++) {
>> +
>> + if (c[i].fd == (ngx_socket_t) -1
>> + || c[i].read == NULL
>> + || c[i].read->accept
>> + || c[i].read->channel
>> + || c[i].read->resolver)
>> + {
>> + continue;
>> + }
>> +
>> + ngx_log_debug1(NGX_LOG_DEBUG_CORE, ev->log, 0,
>> + "*%uA shutdown timeout", c[i].number);
>> +
>> + c[i].close = 1;
>> + c[i].error = 1;
>> +
>> + c[i].read->handler(c[i].read);
>> + }
>> +}
>> diff --git a/src/core/ngx_cycle.h b/src/core/ngx_cycle.h
>> --- a/src/core/ngx_cycle.h
>> +++ b/src/core/ngx_cycle.h
>> @@ -88,6 +88,7 @@ typedef struct {
>> ngx_flag_t master;
>>
>> ngx_msec_t timer_resolution;
>> + ngx_msec_t shutdown_timeout;
>>
>> ngx_int_t worker_processes;
>> ngx_int_t debug_points;
>> @@ -129,6 +130,7 @@ ngx_pid_t ngx_exec_new_binary(ngx_cycle_
>> ngx_cpuset_t *ngx_get_cpu_affinity(ngx_uint_t n);
>> ngx_shm_zone_t *ngx_shared_memory_add(ngx_conf_t *cf, ngx_str_t *name,
>> size_t size, void *tag);
>> +void ngx_set_shutdown_timer(ngx_cycle_t *cycle);
>>
>>
>> extern volatile ngx_cycle_t *ngx_cycle;
>> diff --git a/src/os/unix/ngx_process_cycle.c
>> b/src/os/unix/ngx_process_cycle.c
>> --- a/src/os/unix/ngx_process_cycle.c
>> +++ b/src/os/unix/ngx_process_cycle.c
>> @@ -763,6 +763,7 @@ ngx_worker_process_cycle(ngx_cycle_t *cy
>>
>> if (!ngx_exiting) {
>> ngx_exiting = 1;
>> + ngx_set_shutdown_timer(cycle);
>> ngx_close_listening_sockets(cycle);
>> ngx_close_idle_connections(cycle);
>> }
>> diff --git a/src/os/win32/ngx_process_cycle.c
>> b/src/os/win32/ngx_process_cycle.c
>> --- a/src/os/win32/ngx_process_cycle.c
>> +++ b/src/os/win32/ngx_process_cycle.c
>> @@ -800,6 +800,7 @@ ngx_worker_thread(void *data)
>>
>> if (!ngx_exiting) {
>> ngx_exiting = 1;
>> + ngx_set_shutdown_timer(cycle);
>> ngx_close_listening_sockets(cycle);
>> ngx_close_idle_connections(cycle);
>> }
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>>
>
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
--
*Datong Sun* | Systems Engineer
dat...@cloudflare.com
<https://www.cloudflare.com/>
1 888 99 FLARE | www.cloudflare.com
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
