On 02/17/2012 01:59 AM, Amos Jeffries wrote: > I'm not sure about: > > + // XXX: stale serverConn may remain set even though we are opening > new conn > > ** This should not matter, the next stage will either retry failed > connect using it yet again, or reset serverConn to the newely opened > Comm::Connection (itself). If you want to be certain about that you can > move the "serverConn = temp;" line up out of the if() condition.
I hope it does not matter but it feels very wrong to keep stale information around. Since you think it should work fine, I will set serverConn to temp unconditionally and remove the XXX. serverConn will be nill where that XXX is. > We are more likely to have problems from the local port number of the > serverConn being non-0 on the retries. Since the IP is set Squid will > use bind() to ensure the outgoing IP is retained and bind() does not > like ports which are in TIME_WAIT from previous use. Sorry I missed this > on the last audit. > The place to add serverConn->local.SetPort(0) is retryOrBail() alongside > the new debugs() I do not see port number being reused in basic tests. Perhaps it is reused in some complex configurations, but you do raise a valid concern even if the port number is never reused. Let's clarify what needs to be done here. We do not use serverConn pointer when retrying a failed connection. And after the above modification serverConn will be NULL during retries anyway. I can set serverConn->local port to zero just before setting the serverConn pointer to NULL, but it seems pointless or at least very confusing because it implies we actually want to modify something other than serverConn. We do reuse serverDestinations[0]. Until I read your comment above, I assumed that address remains constant after serverDestinations is built. Now I see that it is not a constant address but a half-baked, fully-baked, or closed connection, depending on the ConnOpener and FwdState state. This implies we may not reuse it safely. I see several options: a) Change FwdState to zero serverDestinations[0].local port before calling Comm::ConnOpener(). This should work if we never pre-set the port elsewhere due to some configuration options/etc. (if we do, we should not zero it). Are there any other fields we should reset?? b) Change Comm::ConnOpener to always zero the local port. This will work only if we never preset the local port for any connection we are about to open. Some ConnOpener code appears to imply that the connection may already be opened before ConnOpener was called, so this may not work at all. Are there any other fields we should reset?? c) Change Comm::ConnOpener to preserve serverDestinations[0] passed to it. This might break some other new code that assumes that ConnOpener modifies its first constructor parameter, I guess. We do not need to guess which fields require a reset (now or in the future). d) Change FwdState to copyDetails() of the serverDestinations[0] "connection" and give Comm::ConnOpener that copy so that ConnOpener can modify the copy as it pleases. Upon call back, serverConn will have the modified value but serverDestinations[0] will be preserved. We do not need to guess which fields require a reset (now or in the future). I guess I would favor (d), but I suspect it goes against your overall Connection design. Do you want me to do (a)? Thank you, Alex.
