On Sun, 18 Jul 2010 19:15:37 -0600, Alex Rousskov <[email protected]> wrote: > On 07/18/2010 04:52 AM, Amos Jeffries wrote: >> This updated my previous submission for the comm connection opener model. >> >> diff since last time: >> >> - removed the multi-path pathways. They are now isolated at the upper >> level. ConnOpener now handles only one Comm::Connection. >> >> - implemented ConnOpener as an AsyncJob child. >> >> >> What has changed from HEAD: >> >> ConnectionDetails objects have been renamed Comm::Connection and been >> extended to hold the FD and Squids' socket flags. > ... >> On COMM_ERR_CONNECT and COMM_TIMEOUT the conn will be !isOpen() or NULL >> and should not be relied on. > > If the connection pointer is NULL, a job opening multiple connections > would not be able to tell which connection just failed. Let's not hide > connection pointer from the initiator.
Understood. I'm not sure exactly why I wrote NULL as a case there. > > * Can Adaptation::Icap::Xaction::closeConnection set connection to NULL? > Yes. Since I got to know AsyncJob a bit better now I think the concept of "connection" pointer in Xaction is maybe a little wrong. Check me on this but here are a few state things I found working with FwdState and the InternalServer from the PAC stuff... In the Xaction code "connection" is meant to indicate both that a connection is being attempted and what that connection is (its FD). right? In the async time between connect being started and connect being completed the FD-state of connection switches from being <0 to being >=0. This starts a race condition between the ConnOpener finishing and any caller abort/timeout handler which may try to close an half-open conn->fd itself. Which would trigger the ConnOpener to open a new socket on it's next call and re-start the connection setup, then die on its AsyncCall callback (for some reason the dialer GetParams fails with a NULL params object if the caller has died but the call is still scheduled without cancelling). The solution I use in the ConnOpener itself (for earlyabort and timeout) is to have the caller hold a AsyncCall::Pointer to the call is told the ConnOpener to make. And use that ptrs NULL / not-NULL state in the abort code to identify if a ConnOpener is running or not. IF a ConnOpener is running then the call (for which we now have a ptr) can be cancelled and the conn 'erased' (Comm::ConnectionPointer set to NULL along with the held AsyncCall::Pointer). This leaves the ConnOpener to finish and no callback happens, ConnOpener completing then re-closes the FD which was attempted. <snip bits I'll reply to when I'm back in front of the code> > > * FwdState::complete may shift paths (via reforward). If that happens, > the following will close a different/new connection or assert/throw (or > even segfault in the current patch): > > fwd->complete(); > - comm_close(fd); > + fwd->conn()->close(); > > Overall, exposing connection pointer to Servers seems wrong, especially > because the pointer may change from one call to another. A Server that > needs that pointer, should store it at initialization time, just like it > stores fd in the current code. > > You have "TODO: store Comm::Connection instead of FD". This is an XXX > and not just a TODO because of the above concerns. Ah, so XXX to you means something other than a personal TODO marker? <snip> > > > That is all I had time for, sorry. > No prob. thats heaps for me to work on tonight :)
