Re: Client-side pconns and r10728

On 08/13/2010 09:30 PM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> On 08/12/2010 03:37 AM, Amos Jeffries wrote:
>>> ------------------------------------------------------------
>>> revno: 10728
>>> committer: Amos Jeffries<squid3_at_treenet.co.nz>
>>> branch nick: trunk
>>> timestamp: Thu 2010-08-12 21:37:14 +1200
>>> message:
>>> Author: Stephen Thorne<stephen_at_thorne.id.au>
>>> Bug 2936: NTLM-Authenticate 407 and Proxy-Connection: Close in same
>>> response.
>>>
>>> Squid default from the days of HTTP/1.0 was to close connections unless
>>> keep-alive was explicitly known. This changes the default to send
>>> keep-alive unless we have a good reason to close.
>>> modified:
>>> src/client_side_reply.cc
>>
>>> === modified file 'src/client_side_reply.cc'
>>> --- a/src/client_side_reply.cc 2010-07-13 14:27:25 +0000
>>> +++ b/src/client_side_reply.cc 2010-08-12 09:37:14 +0000
>>> @@ -1383,6 +1383,9 @@
>>> } else if (fdUsageHigh()&& !request->flags.must_keepalive) {
>>> debugs(88, 3, "clientBuildReplyHeader: Not many unused FDs, can't
>>> keep-alive");
>>> request->flags.proxy_keepalive = 0;
>>> + } else if (request->http_ver.major == 1 && request->http_ver.minor
>>> == 1) {
>>> + debugs(88, 3, "clientBuildReplyHeader: Client is HTTP/1.1, send
>>> keep-alive, no overriding reasons not to");
>>> + request->flags.proxy_keepalive = 1;
>>> }
>>>
>>
>> Persistent connections have been semi-broken since 3.0, but was the
>> above fix discussed somewhere? I think it contradicts the overall flow
>
> On IRC, with detailed run testing by Stephen.
>
>> of the persistency handling code in general and clientSetKeepaliveFlag
>> intent/documentation in particular. I do not know whether it
>> introduces more bugs, but I would not be surprised if it does because
>> the if-statements above the new code do not enumerate "all overriding
>> reasons"!
>>
>> To add insult to the injury, the commit message is also misleading
>> because, bugs notwithstanding, Squid did "send keep-alive unless we
>> had a good reason to close" even before this change.
>
> Not true. The bug is a clear case of 407 being required, but Connection
> headers not being sent *at all*.

You are saying that there is a bug. I am saying that the overall code
logic and flow was correct (i.e., "persistent if at all possible") until
r10728; the commit message implies it was not correct. The two
statements are not contradictory. r10728 broke the overall code logic
but helped with a bug.

>> You may want to test the attached fix instead. I do not know whether
>> it helps with Bug 2936 specifically, but it does fix a bug that smells
>> related to those issues because Bug 2936 test script uses HTTP/1.0
>> messages.
>
> The extended tests on IRC used 1.1 as well. I think via the
> force_http1p1 patch you provided.

Noted. I suspect Stephen will follow up if the code stops working after
r10728 reversal, and we can find the right fix then.

> Bug 2936 results from the *client* closing the connection in the absence
> of keep-alive. It hits worst in 3.1 where we send HTTP/1.0 to the client
> and don't specify keep-alive explicitly.

The proposed fix works on the client side as well, for both HTTP/1.1 and
HTTP/1.0 clients. A similar fix is needed in all Squid3 versions, IIRC.

> The new patch looks logically right. This is a clean section 8.1.3
> clause 3 violation fix by the looks of it.

The "MUST NOT establish a HTTP/1.1 persistent connection with an
HTTP/1.0 client" requirement is kind of related to this, but the bug is
in-between two specs: When talking to a X.Y client, we must use X.Y
rules or reject/tunnel the message. There is no explicit MUST like that.

> * Please add a reference to RFC 2616 section 8.1.3 clause 3 for the if
> statement. ie "MAY keep-alive to 1.1 clients, MUST NOT keep-alive
> default to 1.0 clients.".

I do not think such a comment would be clear in the context of the
if-statement.

> The particular point being that it's based on the client version. Not
> Squid like we currently have.

Yes, that is the point. I will add a comment about it although I think
it belongs to the httpMsgIsPersistent() API.

> - Its not relevant on httpMsgIsPersistent() documentation due to that
> function applying to both server and client conn.

It is relevant because the rules are the same on both sides.
httpMsgIsPersistent() must see the message it is evaluating for
persistency. Squid version is irrelevant for that evaluation.
Misunderstanding of this principle is the primary cause of a series of
wrong httpMsgIsPersistent()-related changes that we are slowly weeding
out now.

Squid version is only relevant for what Connection header to send after
the persistency decision has been made. However, we always send explicit
Connection header even if we do not have to due to protocol defaults for
Squid protocol version. Thus, Squid version is pretty much irrelevant.

> NP: 3.2 and 3.HEAD still contain the 1.1 advertisement to clients. It's
> only gone from the stable release while pending the non-buffered client
> request de-chunking you scheduled for 3.2. Is the timeline still at the
> end-August on that?

Stephen has reported that the initial patch solved his problem so we
should be on track to port it to trunk in August. Please note that it is
not about request de-chunking but response chunking :-). With that
patch, Squid does not close the connection just because the response has
no Content-Length. Instead, Squid chunks the response (subject to
several conditions) and keeps the connection open.

Cheers,

Alex.
Received on Sat Aug 14 2010 - 12:57:31 MDT