<div dir="ltr">Hello!<div><br></div><div>Looks like to me that the original patch does what it's supposed to do (when combined with <a href="http://hg.nginx.org/nginx/rev/3069dd358ba2" rel="noreferrer" target="_blank" style="font-size:12.8px">http://hg.nginx.org/nginx/rev/<wbr>3069dd358ba2</a>). Here is my understanding:</div><div><br></div><div>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 <span style="font-size:12.8px"><font face="monospace, monospace">ngx_shutdown_timer_handler</font> 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.</span></div><div><span style="font-size:12.8px"><br></span></div><div><span style="font-size:12.8px">If all connections were closed before </span><span style="font-family:monospace,monospace;font-size:12.8px">ngx_shutdown_timer_handler</span><span style="font-size:12.8px"> was fired (your case), it is already safe to finish the shutdown and there is no reason to invoke or wait for </span><span style="font-family:monospace,monospace;font-size:12.8px">ngx_shutdown_timer_handler</span> to fire anymore<span style="font-family:monospace,monospace;font-size:12.8px">.</span> The <font face="monospace, monospace">cancelable</font> field makes sure when this happens <font face="monospace, monospace">ngx_shutdown_event</font> does not prevent NGINX from being able to complete the shutdown.</div><div><br></div><div>Seems to me that delaying the shutdown until <font face="monospace, monospace">worker_shutdown_timeout</font> when there is already no active connection left makes little sense.</div><div><br></div><div>Thanks,</div><div>Datong</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 7, 2017 at 11:52 PM, 洪志道 <span dir="ltr"><<a href="mailto:hongzhidao@gmail.com" target="_blank">hongzhidao@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi!<div><br></div><div>It's a good design.</div><div>It seems the worker process is not killed until expired, the time is set by '<span style="font-size:14px">worker_shutdown_timeout'.</span></div><div><span style="font-size:14px">But I think </span><span style="font-size:14px">ngx_shutdown_timer_<wbr>handler will never be called, because of the following deal.</span></div><div><span style="font-size:14px"><br></span></div><div><div><span style="font-size:14px"> if (ngx_exiting) {</span></div><div><span style="font-size:14px"> if (ngx_event_no_timers_left() == NGX_OK) {</span></div><div><span style="font-size:14px"> ngx_log_error(NGX_LOG_NOTICE, cycle->log, 0, "exiting");</span></div><div><span style="font-size:14px"> ngx_worker_process_exit(cycle)<wbr>; // the worker process exits though there are cancelable timers such as </span><span style="font-size:14px">ngx_shutdown_event.</span></div><div><span style="font-size:14px"> }</span></div><div><span style="font-size:14px"> }</span></div></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">I'm not sure it's right?</span></div><div><span style="font-size:14px"><br></span></div><div><div><span style="font-size:14px">diff -r d45072375572 src/core/ngx_cycle.c</span></div><div><span style="font-size:14px">--- a/src/core/ngx_cycle.c<span class="m_-8642361306868347428gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>Tue Mar 07 18:51:17 2017 +0300</span></div><div><span style="font-size:14px">+++ b/src/core/ngx_cycle.c<span class="m_-8642361306868347428gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>Sun Mar 05 17:03:22 2017 +0800</span></div><div><span style="font-size:14px">@@ -1348,7 +1348,6 @@</span></div><div><span style="font-size:14px"> ngx_shutdown_event.handler = ngx_shutdown_timer_handler;</span></div><div><span style="font-size:14px"> ngx_shutdown_event.data = cycle;</span></div><div><span style="font-size:14px"> ngx_shutdown_event.log = cycle->log;</span></div><div><span style="font-size:14px">- ngx_shutdown_event.cancelable = 1;</span></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px"> ngx_add_timer(&ngx_shutdown_<wbr>event, ccf->shutdown_timeout);</span></div><div><span style="font-size:14px"> }</span></div></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">And I think it's better that the shutting down process keep alive until '</span><span style="font-size:14px">worker_shutdown_</span><span style="font-size:14px">timeout'.</span></div><div><span style="font-size:14px"><br></span></div><div><span style="font-size:14px">Thanks.<br>B.R.</span></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">2017-03-08 1:01 GMT+08:00 Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">details: <a href="http://hg.nginx.org/nginx/rev/97c99bb43737" rel="noreferrer" target="_blank">http://hg.nginx.org/nginx/<wbr>rev/97c99bb43737</a><br>
branches:<br>
changeset: 6930:97c99bb43737<br>
user: Maxim Dounin <<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>><br>
date: Tue Mar 07 18:51:16 2017 +0300<br>
description:<br>
Introduced worker_shutdown_timeout.<br>
<br>
The directive configures a timeout to be used when gracefully shutting down<br>
worker processes. When the timer expires, nginx will try to close all<br>
the connections currently open to facilitate shutdown.<br>
<br>
diffstat:<br>
<br>
src/core/nginx.c | 9 ++++++<br>
src/core/ngx_cycle.c | 53 ++++++++++++++++++++++++++++++<wbr>++++++++++<br>
src/core/ngx_cycle.h | 2 +<br>
src/os/unix/ngx_process_<wbr>cycle.c | 1 +<br>
src/os/win32/ngx_process_cycl<wbr>e.c | 1 +<br>
5 files changed, 66 insertions(+), 0 deletions(-)<br>
<br>
diffs (148 lines):<br>
<br>
diff --git a/src/core/nginx.c b/src/core/nginx.c<br>
--- a/src/core/nginx.c<br>
+++ b/src/core/nginx.c<br>
@@ -124,6 +124,13 @@ static ngx_command_t ngx_core_commands[<br>
offsetof(ngx_core_conf_t, rlimit_core),<br>
NULL },<br>
<br>
+ { ngx_string("worker_shutdown_ti<wbr>meout"),<br>
+ NGX_MAIN_CONF|NGX_DIRECT_CONF|<wbr>NGX_CONF_TAKE1,<br>
+ ngx_conf_set_msec_slot,<br>
+ 0,<br>
+ offsetof(ngx_core_conf_t, shutdown_timeout),<br>
+ NULL },<br>
+<br>
{ ngx_string("working_directory"<wbr>),<br>
NGX_MAIN_CONF|NGX_DIRECT_<wbr>CONF|NGX_CONF_TAKE1,<br>
ngx_conf_set_str_slot,<br>
@@ -1014,6 +1021,7 @@ ngx_core_module_create_conf(ng<wbr>x_cycle_t<br>
ccf->daemon = NGX_CONF_UNSET;<br>
ccf->master = NGX_CONF_UNSET;<br>
ccf->timer_resolution = NGX_CONF_UNSET_MSEC;<br>
+ ccf->shutdown_timeout = NGX_CONF_UNSET_MSEC;<br>
<br>
ccf->worker_processes = NGX_CONF_UNSET;<br>
ccf->debug_points = NGX_CONF_UNSET;<br>
@@ -1042,6 +1050,7 @@ ngx_core_module_init_conf(ngx_<wbr>cycle_t *c<br>
ngx_conf_init_value(ccf->daem<wbr>on, 1);<br>
ngx_conf_init_value(ccf->mast<wbr>er, 1);<br>
ngx_conf_init_msec_value(ccf-<wbr>>timer_resolution, 0);<br>
+ ngx_conf_init_msec_value(ccf-><wbr>shutdown_timeout, 0);<br>
<br>
ngx_conf_init_value(ccf->work<wbr>er_processes, 1);<br>
ngx_conf_init_value(ccf->debu<wbr>g_points, 0);<br>
diff --git a/src/core/ngx_cycle.c b/src/core/ngx_cycle.c<br>
--- a/src/core/ngx_cycle.c<br>
+++ b/src/core/ngx_cycle.c<br>
@@ -15,6 +15,7 @@ static ngx_int_t ngx_init_zone_pool(ngx_<br>
ngx_shm_zone_t *shm_zone);<br>
static ngx_int_t ngx_test_lockfile(u_char *file, ngx_log_t *log);<br>
static void ngx_clean_old_cycles(ngx_event<wbr>_t *ev);<br>
+static void ngx_shutdown_timer_handler(ngx<wbr>_event_t *ev);<br>
<br>
<br>
volatile ngx_cycle_t *ngx_cycle;<br>
@@ -22,6 +23,7 @@ ngx_array_t ngx_old_cycles;<br>
<br>
static ngx_pool_t *ngx_temp_pool;<br>
static ngx_event_t ngx_cleaner_event;<br>
+static ngx_event_t ngx_shutdown_event;<br>
<br>
ngx_uint_t ngx_test_config;<br>
ngx_uint_t ngx_dump_config;<br>
@@ -1333,3 +1335,54 @@ ngx_clean_old_cycles(ngx_event<wbr>_t *ev)<br>
ngx_old_cycles.nelts = 0;<br>
}<br>
}<br>
+<br>
+<br>
+void<br>
+ngx_set_shutdown_timer(ngx_cy<wbr>cle_t *cycle)<br>
+{<br>
+ ngx_core_conf_t *ccf;<br>
+<br>
+ ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx, ngx_core_module);<br>
+<br>
+ if (ccf->shutdown_timeout) {<br>
+ ngx_shutdown_event.handler = ngx_shutdown_timer_handler;<br>
+ ngx_shutdown_event.data = cycle;<br>
+ ngx_shutdown_event.log = cycle->log;<br>
+ ngx_shutdown_event.cancelable = 1;<br>
+<br>
+ ngx_add_timer(&ngx_shutdown_ev<wbr>ent, ccf->shutdown_timeout);<br>
+ }<br>
+}<br>
+<br>
+<br>
+static void<br>
+ngx_shutdown_timer_handler(ng<wbr>x_event_t *ev)<br>
+{<br>
+ ngx_uint_t i;<br>
+ ngx_cycle_t *cycle;<br>
+ ngx_connection_t *c;<br>
+<br>
+ cycle = ev->data;<br>
+<br>
+ c = cycle->connections;<br>
+<br>
+ for (i = 0; i < cycle->connection_n; i++) {<br>
+<br>
+ if (c[i].fd == (ngx_socket_t) -1<br>
+ || c[i].read == NULL<br>
+ || c[i].read->accept<br>
+ || c[i].read->channel<br>
+ || c[i].read->resolver)<br>
+ {<br>
+ continue;<br>
+ }<br>
+<br>
+ ngx_log_debug1(NGX_LOG_DEBUG_C<wbr>ORE, ev->log, 0,<br>
+ "*%uA shutdown timeout", c[i].number);<br>
+<br>
+ c[i].close = 1;<br>
+ c[i].error = 1;<br>
+<br>
+ c[i].read->handler(c[i].read);<br>
+ }<br>
+}<br>
diff --git a/src/core/ngx_cycle.h b/src/core/ngx_cycle.h<br>
--- a/src/core/ngx_cycle.h<br>
+++ b/src/core/ngx_cycle.h<br>
@@ -88,6 +88,7 @@ typedef struct {<br>
ngx_flag_t master;<br>
<br>
ngx_msec_t timer_resolution;<br>
+ ngx_msec_t shutdown_timeout;<br>
<br>
ngx_int_t worker_processes;<br>
ngx_int_t debug_points;<br>
@@ -129,6 +130,7 @@ ngx_pid_t ngx_exec_new_binary(ngx_cycle_<br>
ngx_cpuset_t *ngx_get_cpu_affinity(ngx_uint<wbr>_t n);<br>
ngx_shm_zone_t *ngx_shared_memory_add(ngx_con<wbr>f_t *cf, ngx_str_t *name,<br>
size_t size, void *tag);<br>
+void ngx_set_shutdown_timer(ngx_cyc<wbr>le_t *cycle);<br>
<br>
<br>
extern volatile ngx_cycle_t *ngx_cycle;<br>
diff --git a/src/os/unix/ngx_process_cycl<wbr>e.c b/src/os/unix/ngx_process_cycl<wbr>e.c<br>
--- a/src/os/unix/ngx_process_cycl<wbr>e.c<br>
+++ b/src/os/unix/ngx_process_cycl<wbr>e.c<br>
@@ -763,6 +763,7 @@ ngx_worker_process_cycle(ngx_c<wbr>ycle_t *cy<br>
<br>
if (!ngx_exiting) {<br>
ngx_exiting = 1;<br>
+ ngx_set_shutdown_timer(cycle);<br>
ngx_close_listening_sockets(c<wbr>ycle);<br>
ngx_close_idle_connections(cy<wbr>cle);<br>
}<br>
diff --git a/src/os/win32/ngx_process_cyc<wbr>le.c b/src/os/win32/ngx_process_cyc<wbr>le.c<br>
--- a/src/os/win32/ngx_process_cyc<wbr>le.c<br>
+++ b/src/os/win32/ngx_process_cyc<wbr>le.c<br>
@@ -800,6 +800,7 @@ ngx_worker_thread(void *data)<br>
<br>
if (!ngx_exiting) {<br>
ngx_exiting = 1;<br>
+ ngx_set_shutdown_timer(cycle);<br>
ngx_close_listening_sockets(c<wbr>ycle);<br>
ngx_close_idle_connections(cy<wbr>cle);<br>
}<br>
______________________________<wbr>_________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/mailm<wbr>an/listinfo/nginx-devel</a><br>
</blockquote></div><br></div>
</div></div><br>______________________________<wbr>_________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" rel="noreferrer" target="_blank">http://mailman.nginx.org/<wbr>mailman/listinfo/nginx-devel</a><br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><p style="font-family:Helvetica;font-size:12px;color:rgb(64,64,64)"><b>Datong Sun</b> | Systems Engineer<br><a href="mailto:datong@cloudflare.com" style="color:rgb(47,123,191)" target="_blank">datong@cloudflare.com</a></p><a href="https://www.cloudflare.com/" style="font-family:Times;font-size:medium" target="_blank"><div style="background-image:url("https://www.cloudflare.com/img/signature-cloud.png");width:200px;height:30px;margin-right:20px;margin-top:20px"></div></a><p style="font-family:Helvetica;font-size:12px;color:rgb(64,64,64)">1 888 99 FLARE | <a href="https://www.cloudflare.com/" style="color:rgb(47,123,191)" target="_blank">www.cloudflare.com</a></p></div></div>
</div>