[PATCH] HttpMsg::persistent (Was: Client-side pconns and r10728)

Hello,

     After sorting out misunderstandings with Amos on IRC, I would like
to propose another version of the patch. It should be more clear.

For v3.1, I would still recommend the earlier one-line change because it
is less intrusive.

----------------
Moved httpMsgIsPersistent(version, headers) to HttpMsg::persistent(void).

This move makes it clear that the logic applies only to the message
being examined and not some irrelevant information such as HTTP version
supported by Squid.

Side-effects:

   - In trunk, Squid stops using persistent connections with HTTP/1.0
     clients that do not send "Connection: keep-alive".

   - In v3.1, Squid starts using persistent connections with HTTP/1.1
     clients that do not send "Connection: close". This patch is not
     recommended for v3.1 though because it is too intrusive.
     Port the previously posted take1 instead. It has the same effect.

   - HttpReply now sets HttpMsg::http_ver member. It is not clear whether
     that member was ever used for HttpReplies though.

--------------

The patch adds a local variable to show that the original
httpMsgIsPersistent() implementation has not changed at all. That
variable should be removed before committing and the "header" member
used instead.

Thank you,

Alex.
P.S. IIRC, Squid v3.0 needs the same kind of fix and removal of #ifdefs
from httpMsgIsPersistent().

On 08/13/2010 07:42 PM, 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
> 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.
>
> Can we revert the above change, please?
>
>
> 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.
>
> Thank you,
>
> Alex.