Inconsistent order for ngx_destroy_pool() and ngx_close_connection()

[Ruslan Ermilov]

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.

> 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():

: @@ -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().