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