On 05/08/2014 09:35 PM, squ...@treenet.co.nz wrote: > I think moving the stats is a good idea.
OK, I will work on that. > Ive not yet checked your commit but at all,9 debug i also hit the debugs > array dereference before noteUses I now added protection for the debugs() call as well (r13406). Thank you, Alex. > ------------------------------------------------------------------------ > From : Alex Rousskov > To : squid-dev@squid-cache.org; > Subject : 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. >
