On 18/02/2012 12:36 p.m., Alex Rousskov wrote:
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.
If it has closed already FD value is invalid we can re-use most of the
other settings inside it. Whether we want to re-use the exact conn
object or a clone is up to the context. Re-using the same one means we
have to reset and loose some details logging may have wanted to keep for
the connection attempt history, FwdState does not keep such a history so
no problem there at this point.
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??
We *can't* pre-set the port for any outgoing connections, due to the
same bind() problem. Even TPROXY outgoings only copy the address over.
The only addressing detail we need to reset is the local outgoing port.
I'm not sure if its relevant outside of FTP Data connections but
potentially the flags bitmap member might need the COMM_REUSEADDR bit
un-set.
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??
If it is done by ConnOpener then no. Only that port is relevant.
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).
Um. Possible, but inefficient. The current code receives back "a" open
Comm::Connection for use. Any checking that it was the same one passed
in is arbitrary. But this does mean adding extra allocation/deallocation
around the connection setup. Which this design was seeking to avoid.
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).
Also, possible. And better than (c), since it only allocates on these
race cases.
I guess I would favor (d), but I suspect it goes against your overall
Connection design. Do you want me to do (a)?
(a) or (b) seem right without re-designing things.
(b) would appear to be the right thing to do for the future. But affects
a wider range of code than (a) and needs equivaently more testing. So
its up to you which you want to go for now.
Amos
