Re: Segmentation fault in FwdState::serverClosed

On 05/08/2014 11:49 AM, Amos Jeffries wrote:

>>> Something is still calling serverConn->close() directly

In your particular trace, HttpStateData::continueAfterParsingHeader()
does that, but there are many other methods doing the same thing.
AFAICT, the idea is that the close handlers will clean up during
connection closure, including the FwdState close handler. While the
whole FwdState/Server integration is an ugly mess, that close() calling
code in ServerStateData and its kids is not at fault here.

> instead of via the FwdState::closeServer() method.

I am guessing you are thinking about FwdState::closeServerConnection().
It is a private method encapsulating repeated code internal to FwdState.
It was not meant to be, cannot, and should not be used outside FwdState
itself "as is".

>> Do you know how a connection close callback (fwdServerClosedWrapper) can
>> be called with fd set to -1? It feels like there is a shared Connection
>> object that two different jobs are updating/closing independently,
>> stepping on each other's toes.

Actually, I bet CommCloseCbParams.fd is just never set.
commCallCloseHandlers() is supposed to set it, but it does not.

This is a bug, but I suspect most close handlers do not care about the
descriptor value so they continue to work fine. r13388 (standy
connections) was not that lucky. I added the following check to update
the pconn stats for the closing connection:

> if (serverConnection()->fd == fd) // should be, but not critical to assert
> fwdPconnPool->noteUses(fd_table[fd].pconn.uses);

That new check does not work as intended because both fds are -1 in this
case: the first one is reset by http.cc code that shares Connection
object with FwdState and the second is never set by Comm.

The above noteUses() call was moved from Comm (where it did not belong
in the first place and caused other troubles after we added non-static
pconn pools). Here is that old Comm code calling noteUses() for static
pconn pools:

> commCallCloseHandlers(fd);
>
> - if (F->pconn.uses && F->pconn.pool)
> - F->pconn.pool->noteUses(F->pconn.uses);

I have committed a short-term fix for this bug (r13404). Here is updated
FwdState::serverClosed():

> - if (serverConnection()->fd == fd) // should be, but not critical to assert
> + if (fd >= 0 && serverConnection()->fd == fd) // XXX: fd is often -1 here
> fwdPconnPool->noteUses(fd_table[fd].pconn.uses);

This change should prevent serverClosed() segfaults, but it is wrong.

For the correct solution, I am considering moving pconn.uses from
fd_table to Connection. What do you think?

Thank you,

Alex.
Received on Thu May 08 2014 - 22:48:08 MDT