[PATCH] Ensured SIGQUIT deletes listening UNIX socket files.

[Thibault Charbonnier]

Hello Maxim,

Any thoughts on the latest version of this patch?

Cheers,

On 2/27/20 4:23 PM, Thibault Charbonnier wrote:
> On 2/27/20 7:24 AM, Maxim Dounin wrote:
>> Have you checked what happens during binary upgrade with your 
>> patch?
> 
> Good call, I gave it a thought but did not bother checking, thanks for
> sharing the previous patch. This one is slightly simpler.
> 
> Below is a new version of the patch covering binary upgrade edge-cases
> by relying on the existence of the nginx.oldpid file.
> 
> Also attached to this email is a file I used as a test suite covering
> the behavior of this patch with SIGQUIT, SIGTERM, and binary upgrade
> scenarios.
> 
> # HG changeset patch
> # User Thibault Charbonnier <thibaultcha at me.com>
> # Date 1582764433 28800
> #      Wed Feb 26 16:47:13 2020 -0800
> # Node ID ec619d02801b925b4dad51515fb9668c1d993418
> # 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 ec619d02801b 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
> @@ -1023,6 +1023,10 @@
>      ngx_uint_t         i;
>      ngx_listening_t   *ls;
>      ngx_connection_t  *c;
> +#if (NGX_HAVE_UNIX_DOMAIN)
> +    ngx_core_conf_t   *ccf;
> +    ngx_fd_t           fd;
> +#endif
> 
>      if (ngx_event_flags & NGX_USE_IOCP_EVENT) {
>          return;
> @@ -1067,10 +1071,22 @@
>          }
> 
>  #if (NGX_HAVE_UNIX_DOMAIN)
> +        ccf = (ngx_core_conf_t *) ngx_get_conf(cycle->conf_ctx,
> ngx_core_module);
> +
> +        fd = ngx_open_file(ccf->oldpid.data, NGX_FILE_RDONLY,
> NGX_FILE_OPEN, 0);
> +
> +        if (fd != NGX_INVALID_FILE) {
> +            if (ngx_close_file(fd) == NGX_FILE_ERROR) {
> +                ngx_log_error(NGX_LOG_ALERT, cycle->log, ngx_errno,
> +                              ngx_close_file_n " \"%s\" failed",
> +                              ccf->oldpid.data);
> +            }
> +        }
> 
>          if (ls[i].sockaddr->sa_family == AF_UNIX
>              && ngx_process <= NGX_PROCESS_MASTER
> -            && ngx_new_binary == 0)
> +            && ngx_new_binary == 0
> +            && fd == NGX_INVALID_FILE)
>          {
>              u_char *name = ls[i].addr_text.data + sizeof("unix:") - 1;
> 
> diff -r 4f18393a1d51 -r ec619d02801b 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;
>  >
>> Previous attempt to fix this was here:
>>
>> http://mailman.nginx.org/pipermail/nginx-devel/2016-December/009207.html
>> http://mailman.nginx.org/pipermail/nginx-devel/2016-December/009208.html
>>
>> Yet it failed to address binary upgrade case properly, see here:
>>
>> http://mailman.nginx.org/pipermail/nginx-devel/2016-December/009239.html
>>
> 
>      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;
>          }
> 
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>