Re: Client-side pconns and r10728

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*.

>
> Can we revert the above change, please?
>

Okay. Done for now until this is sorted out.

>
> 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.

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 new patch looks logically right. This is a clean section 8.1.3
clause 3 violation fix by the looks of it.

* 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.".
The particular point being that it's based on the client version. Not
Squid like we currently have.
  - Its not relevant on httpMsgIsPersistent() documentation due to that
function applying to both server and client conn.

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?

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.6
   Beta testers wanted for 3.2.0.1