Client-side pconns and r10728

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.