Re: [PATCH] Close idle client connections associated with closed idle pinned connections

On 08/20/2013 03:14 AM, Amos Jeffries wrote:
> On 17/08/2013 12:45 p.m., Alex Rousskov wrote:
>> Hello,
>>
>> Squid was not monitoring idle persistent connections pinned to
>> servers. Squid would discover that the pinned server connection is
>> closed only after receiving a new request on the idle client connection
>> and trying to write that request to the server. In such cases, Squid
>> propagates the pinned connection closure to the client (as it should).
>>
>> Chrome and, to a lesser extent, Firefox handle such races by opening a
>> new connection to Squid and resending the failed [idempotent] request
>> transparently to the user. However, IE usually displays an error page to
>> the user.
>>
>> While some pconn races cannot be avoided, without monitoring idle
>> pconns, Squid virtually guaranteed such a race in environments where
>> origin server idle connection timeout is smaller than client/Squid
>> timeouts and users are revisiting server pages in the window between
>> those two timeouts.
>>
>> With this patch, Squid monitors idle pinned connections similar to idle
>> connections in the pconn pool and closes the corresponding idle client
>> connection to keep the two sides in sync (to the extent possible).
>>
>> The attached patches are for a v3.3-based branch and trunk.

> Audit:
>
> * ConnStateData::clientPinnedConnectionRead please use
> !getCurrentContext() instead of using !currentobject.

OK.

> * I note one side effect of this monitoring:
> If the server is behaving badly and sending bytes back to Squid during
> the idle period before a new client request has been received and
> disabled monitoring it will result in the connection closing.

Yes, this change makes Squid behavior for pinned and unpinned idle
pconns consistent. They will be closed if server speaks first. I hope
this consistency is a good thing. However, I agree that closing idle
connections under those conditions might be wrong in some cases. I
wonder whether an SSL server might decide to renegotiate something, for
example. I should have documented this concern explicitly, sorry.

We know the current code does not work well in the discussed
environments, and that the proposed change fixes that known problem. I
do not know of any specific cases where this change breaks something. I
think we should take this risk. If it turns out that this change does
more harm than good, then it can be reverted or improved once the actual
problem case becomes known.

> I am in two minds about whether that is a good thing or not. Previously
> server sending whitespace would have been a form of TCP keep-alive and
> ignored silently by the loose parser.

Yes, except in the common case of a regular idle pconn:
IdleConnList::Read() closes those unconditionally.

> But it also provided an avenue for
> cache poisoning and corruption by malicious servers if they chose to
> send non-whitespace.
>
> Overall I'm okay with this going in. +1.

Sounds good. I will wait a little more to give others a chance to voice
their concerns about closing idle pinned connections when server speaks
first.

Thank you,

Alex.
Received on Tue Aug 20 2013 - 17:29:25 MDT