Re: [PATCH] Do not send unretriable requests on reused pinned connections

On 11/30/2012 05:31 PM, Henrik Nordström wrote:
> fre 2012-11-30 klockan 15:30 -0700 skrev Alex Rousskov:
>
>> Squid is sending POST requests on reused pinned connections, and
>> some of those requests fail due to a pconn race, with no possibility for
>> a retry.
>
> Yes... and we have to for NTLM, TPROXY and friends or they get in a bit
> of trouble from connection state mismatch.

Those connections that cannot be repinned should not have canRePin flag
set IIRC. The patch does not change their behavior -- they will continue
to be reused because the last "no choice but to risk it" condition will
be true for them:

>> + // reuse a connection if it is safe or if we have no other choice
>> + if (pconnRace != racePossible || // unused connections are safe
>> + checkRetriable() || // we can retry if there is a race
>> + !request->flags.canRePin) { // we have no other choice

> If sending the request fails we should propagate this to the client by
> resetting the client connection and let the client retry.

yes, except when the connection can be repinned safely. Such a
connection (and only such connection) should not be reused for
non-retriable requests.

>> Squid uses the dedicated pinned server connection instead.
>> This bypasses pconn race controls even though Squid may be essentially
>> reusing an idle HTTP connection and, hence, may experience the same kind
>> of race conditions.
>
> Yes..
>
>> However, connections that were just pinned, without sending any
>> requests, are not "essentially reused idle pconns" so we must be careful
>> to allow unretriable requests on freshly pinned connections.
>
> ?

I am not sure what you are asking about, but I can try to rephrase: This
bug is difficult to fix because some pinned connections should be reused
and some should not be. Pinned connections that can be re-pinned but
have not had any HTTP requests sent on them should be reused, even for
unretriable requests. SslBump creates such connections in forward.cc
when Squid connects to the origin server to peak at the server
certificate. Since no HTTP requests were sent on such connections at the
decision time, this is not really a reuse even though it looks like one
in all other aspects.

>> The same logic applies to pinned connection outside SslBump.
>
> Which it quite likely the wrong thing to do. See above.

Does the !flags.canRePin exception address your concern?

Thank you,

Alex.
Received on Sat Dec 01 2012 - 06:07:46 MST