Alex Rousskov wrote:
On 08/12/2010 03:37 AM, Amos Jeffries wrote:
------------------------------------------------------------
revno: 10728
committer: Amos Jeffries<[email protected]>
branch nick: trunk
timestamp: Thu 2010-08-12 21:37:14 +1200
message:
Author: Stephen Thorne<[email protected]>
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
