Re: [PATCH] Retry requests that failed due to a pconn race

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.
Received on Fri Feb 17 2012 - 23:36:33 MST