Hi there,

On Mon, Apr 27, 2020 at 04:26:31PM -0700, Thibault Charbonnier wrote:
> On 4/25/20 6:12 PM, Maxim Dounin wrote:
> > A better approach might be to check parent's pid instead, much
> > like we do when handling the changebin signal on unix (see
> > src/os/unix/ngx_process.c).
>
> Great! Thanks for the suggestion. Below is a revised approach for the
> patch (also attached to this email) which passes all of the test cases
> listed in my previous test file at the start of this thread:
>
> # HG changeset patch
> # User Thibault Charbonnier <thibaultcha@me.com>
> # Date 1582764433 28800
> # Wed Feb 26 16:47:13 2020 -0800
> # Node ID 8d781bac6c4feebb2d1ea3f4e6df76d71f74e43b
> # Parent 4f18393a1d51bce6103ea2f1b2587900f349ba3d
> Ensured SIGQUIT deletes listening UNIX socket files.
>
> Prior to this patch, the SIGQUIT signal handling (graceful shutdown) did not
> remove UNIX socket files since ngx_master_process_cycle reimplemented listening
> socket closings in lieu of using ngx_close_listening_sockets.
>
> Since ngx_master_process_exit will call the aforementioned
> ngx_close_listening_sockets, we can remove the custom implementation and now
> expect listening sockets to be closed properly by ngx_close_listening_sockets
> instead.
>
> This fixes the trac issue #753 (https://trac.nginx.org/nginx/ticket/753).
>
> diff -r 4f18393a1d51 -r 8d781bac6c4f src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c Thu Feb 20 16:51:07 2020 +0300
> +++ b/src/core/ngx_connection.c Wed Feb 26 16:47:13 2020 -0800
> @@ -1070,7 +1070,8 @@
>
> if (ls[i].sockaddr->sa_family == AF_UNIX
> && ngx_process <= NGX_PROCESS_MASTER
> - && ngx_new_binary == 0)
> + && ngx_new_binary == 0
> + && ngx_getppid() != ngx_parent)
> {
> u_char *name = ls[i].addr_text.data + sizeof("unix:") - 1;
>
> diff -r 4f18393a1d51 -r 8d781bac6c4f src/os/unix/ngx_process_cycle.c
> --- a/src/os/unix/ngx_process_cycle.c Thu Feb 20 16:51:07 2020 +0300
> +++ b/src/os/unix/ngx_process_cycle.c Wed Feb 26 16:47:13 2020 -0800
> @@ -77,12 +77,11 @@
> u_char *p;
> size_t size;
> ngx_int_t i;
> - ngx_uint_t n, sigio;
> + ngx_uint_t sigio;
> sigset_t set;
> struct itimerval itv;
> ngx_uint_t live;
> ngx_msec_t delay;
> - ngx_listening_t *ls;
> ngx_core_conf_t *ccf;
>
> sigemptyset(&set);
> @@ -205,16 +204,6 @@
> ngx_signal_worker_processes(cycle,
> ngx_signal_value(NGX_SHUTDOWN_SIGNAL));
>
> - ls = cycle->listening.elts;
> - for (n = 0; n < cycle->listening.nelts; n++) {
> - if (ngx_close_socket(ls[n].fd) == -1) {
> - ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_socket_errno,
> - ngx_close_socket_n " %V failed",
> - &ls[n].addr_text);
> - }
> - }
> - cycle->listening.nelts = 0;
> -
> continue;
> }
>

Thanks for your patch.

Unfortunately, it would break removing of UNIX-domain socket files
when nginx is run with "daemon off".

It'd also add a regression that the master process will not remove
the UNIX-domain socket files until after all worker processes have
exited (this has been fixed in 0.1.40).

The committed fixes:
http://hg.nginx.org/nginx/rev/9c038f5e0464
http://hg.nginx.org/nginx/rev/7cbf6389194b
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel