Inconsistent order for ngx_destroy_pool() and ngx_close_connection()

[Ruslan Ermilov]

On Mon, Oct 03, 2016 at 11:36:19PM +1100, Lawrence Stewart wrote:
> Hi Ruslan,
> 
> On Mon, Oct 3, 2016 at 6:45 PM, Ruslan Ermilov <ru at nginx.com> wrote:
> 
> > On Mon, Oct 03, 2016 at 03:37:38PM +1100, Lawrence Stewart wrote:
> > > Hi all,
> > >
> > > I need to pull kernel data using getsockopt() for a connection prior to
> > the
> > > socket being closed. A pool cleanup handler registered with the
> > > ngx_connection_t's pool would seem to be the right way to do this, but
> > > ngx_http_close_connection() calls ngx_close_connection() prior to
> > > ngx_destroy_pool(), which means my cleanup handler sees a closed socket.
> > >
> > > A look through ngx_close_connection() and the functions it calls would
> > lead
> > > me to believe it should be safe (and possible even desirable in an
> > ordering
> > > sense) to destroy the connection's pool prior to calling
> > > ngx_close_connection().
> >
> > If we talk about client connections, then c->log is allocated from
> > c->pool, and c->read->log and c->write->log also use this log object,
> > see ngx_event_accept().
> >
> > A look through ngx_close_connection() shows that nearly every call
> > it makes: ngx_log_error, ngx_del_timer, ngx_del_conn, ngx_del_event,
> > ngx_delete_posted_event, and ngx_reusable_connection need c->log
> > which would be destroyed if c->pool was destroyed.
> >
> 
> Thanks, I missed this log allocation detail.
> 
> 
> >
> > > There is an existence proof in the nginx tree that
> > > this is reasonable to do (see [1]) and there is also a comment in
> > > ngx_close_connection() that implies it expects that the connection's pool
> > > has already been destroyed i.e.
> > >
> > > /* we use ngx_cycle->log because c->log was in c->pool */
> >
> > This comment seems outdated since rev. c52408583801 which removed
> > the ngx_destroy_pool(c->pool) call from ngx_close_connection():
> >
> >
> Ah, ok.
> 
> 
> > : @@ -388,8 +387,6 @@ void ngx_close_connection(ngx_connection
> > :      c->fd = (ngx_socket_t) -1;
> > :      c->data = NULL;
> > :
> > : -    ngx_destroy_pool(c->pool);
> > : -
> > :      if (ngx_close_socket(fd) == -1) {
> > :
> > :          /* we use ngx_cycle->log because c->log was in c->pool */
> >
> > > Are there some subtleties I'm missing, or is it correct and reasonable to
> > > always call ngx_destroy_pool() on a connection's pool prior to calling
> > > ngx_close_connection()?
> >
> > On the contrary, the upstream connection's pool is destroyed
> > before calling ngx_close_connection(), see ngx_http_upstream_next()
> > and ngx_http_upstream_finalize_request().  This is safe because
> > the upstream connection reuses the client connection's log, see
> > ngx_http_upstream_connect().
> >
> 
> Ah, I missed this detail too. Thanks for the pointer.
> 
> So given that my idea is a fail, what is the recommended way to hook
> connection tear down from a module so that I can achieve my goal?

See if ngx_http_cleanup_add() fits your needs.