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<[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
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.
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. Use take1 instead.
- HttpReply now sets HttpMsg::http_ver member. It is not clear whether
that member was ever used for HttpReplies though.
=== modified file 'src/HttpHeader.h'
--- src/HttpHeader.h 2010-03-05 07:10:40 +0000
+++ src/HttpHeader.h 2010-08-14 15:13:40 +0000
@@ -40,7 +40,6 @@
/* class forward declarations */
-class HttpVersion;
class HttpHdrContRange;
class HttpHdrCc;
class HttpHdrSc;
@@ -278,8 +277,6 @@
extern int httpHeaderParseQuotedString (const char *start, String *val);
SQUIDCEXTERN int httpHeaderHasByNameListMember(const HttpHeader * hdr, const char *name, const char *member, const char separator);
SQUIDCEXTERN void httpHeaderUpdate(HttpHeader * old, const HttpHeader * fresh, const HttpHeaderMask * denied_mask);
-int httpMsgIsPersistent(HttpVersion const &http_ver, const HttpHeader * hdr);
-
SQUIDCEXTERN void httpHeaderCalcMask(HttpHeaderMask * mask, http_hdr_type http_hdr_type_enums[], size_t count);
#endif /* SQUID_HTTPHEADER_H */
=== modified file 'src/HttpMsg.cc'
--- src/HttpMsg.cc 2010-03-10 12:50:39 +0000
+++ src/HttpMsg.cc 2010-08-14 15:18:07 +0000
@@ -316,11 +316,10 @@
content_length = clen;
}
-/* returns true if connection should be "persistent"
- * after processing this message */
-int
-httpMsgIsPersistent(HttpVersion const &http_ver, const HttpHeader * hdr)
+bool
+HttpMsg::persistent() const
{
+ const HttpHeader *hdr = &header; // XXX: diff-minimizer; remove on commit
if ((http_ver.major >= 1) && (http_ver.minor >= 1)) {
/*
* for modern versions of HTTP: persistent unless there is
=== modified file 'src/HttpMsg.h'
--- src/HttpMsg.h 2010-01-01 21:16:57 +0000
+++ src/HttpMsg.h 2010-08-14 15:13:40 +0000
@@ -61,6 +61,14 @@
/// [re]sets Content-Length header and cached value
void setContentLength(int64_t clen);
+ /**
+ * \retval true the message sender asks to keep the connection open.
+ * \retval false the message sender will close the connection.
+ *
+ * Factors other than the headers may result in connection closure.
+ */
+ bool persistent() const;
+
public:
HttpVersion http_ver;
=== modified file 'src/HttpReply.cc'
--- src/HttpReply.cc 2010-07-13 16:43:00 +0000
+++ src/HttpReply.cc 2010-08-14 15:13:40 +0000
@@ -377,12 +377,13 @@
{
HttpMsg::hdrCacheInit();
+ http_ver = sline.version;
content_length = header.getInt64(HDR_CONTENT_LENGTH);
date = header.getTime(HDR_DATE);
last_modified = header.getTime(HDR_LAST_MODIFIED);
surrogate_control = header.getSc();
content_range = header.getContRange();
- keep_alive = httpMsgIsPersistent(sline.version, &header);
+ keep_alive = persistent() ? 1 : 0;
const char *str = header.getStr(HDR_CONTENT_TYPE);
if (str)
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2010-08-07 14:22:54 +0000
+++ src/client_side.cc 2010-08-14 15:16:18 +0000
@@ -735,18 +735,14 @@
clientSetKeepaliveFlag(ClientHttpRequest * http)
{
HttpRequest *request = http->request;
- const HttpHeader *req_hdr = &request->header;
debugs(33, 3, "clientSetKeepaliveFlag: http_ver = " <<
request->http_ver.major << "." << request->http_ver.minor);
debugs(33, 3, "clientSetKeepaliveFlag: method = " <<
RequestMethodStr(request->method));
- /* We are HTTP/1.1 facing clients now*/
- HttpVersion http_ver(1,1);
-
- if (httpMsgIsPersistent(http_ver, req_hdr))
- request->flags.proxy_keepalive = 1;
+ // TODO: move to HttpRequest::hdrCacheInit, just like HttpReply.
+ request->flags.proxy_keepalive = request->persistent() ? 1 : 0;
}
static int
