Inconsistent order for ngx_destroy_pool() and ngx_close_connection()

[Lawrence Stewart]

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?

Cheers,
Lawrence
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20161003/46ae513b/attachment.html>