On Wed, Feb 25, 2004, Henrik Nordstrom wrote:
> On Tue, 24 Feb 2004, Adrian Chadd wrote:
>
> > It doesn't look like a "trivial" fix. Would you mind if I committed
> > the current work, sans re-working the do_next_read flag, so it gets
> > some testing? I'm trying to get squid-3 stable before I jump in
> > to try and improve someo f the code.
>
> Go ahead. The code is a lot better with your patch than before.
>
Heh. Its just about to become a whole heap sillier.
> But please verify that the keep-alives work in all cases. With the split
> logics this is not so simple to verify by inspecting the patch.
Ok. I've tested it and, thus far, I think I've got all the cases
right. I'm having trouble convincing nc to actaully _do_ half-closed
connections - instead, I have to ctrl-C it partway through a request.
It exposed two bugs:
* clientParseRequest(), my creation, was returning true even if a request
wasn't successfully parsed.
* A forced check, at the end of clientReadRequest(), to see whether
we should close the request. I don't think this kind of check
belongs in clientParseRequest(). Without it, a FD would spend 30
seconds being checked for half-closed even with no pending requests
or a full request in the buffer.
Here's the patch. I'd appreciate some external testing.
Adrian
Index: client_side.cc
===================================================================
RCS file: /server/cvs-server/squid/squid3/src/client_side.cc,v
retrieving revision 1.668
diff -u -r1.668 client_side.cc
--- client_side.cc 22 Dec 2003 10:28:49 -0000 1.668
+++ client_side.cc 26 Feb 2004 06:47:24 -0000
@@ -119,6 +119,8 @@
static CWCB clientWriteComplete;
static IOWCB clientWriteBodyComplete;
static IOCB clientReadRequest;
+static bool clientParseRequest(ConnStateData::Pointer conn, bool &do_next_read);
+static void clientAfterReadingRequests(int fd, ConnStateData::Pointer &conn, int
do_next_read);
static PF connStateFree;
static PF requestTimeout;
static PF clientLifetimeTimeout;
@@ -1307,16 +1309,38 @@
ClientSocketContext::keepaliveNextRequest()
{
ConnStateData::Pointer conn = http->getConn();
+ bool do_next_read = false;
debug(33, 3) ("ClientSocketContext::keepaliveNextRequest: FD %d\n", conn->fd);
connIsFinished();
+ if (Config.onoff.half_closed_clients && clientParseRequest(conn, do_next_read)) {
+ debug(33, 3) ("clientSocketContext::keepaliveNextRequest: FD %d: parsing
next request from buffer\n", conn->fd);
+ return;
+ }
+
+ /*
+ * Either we need to kick-start another read or, if we have
+ * a half-closed connection, kill it after the last request.
+ * This saves waiting for half-closed connections to finished being
+ * half-closed _AND_ then, sometimes, spending "Timeout" time in
+ * the keepalive "Waiting for next request" state.
+ */
+ if (commIsHalfClosed(conn->fd) && (conn->getConcurrentRequestCount() == 0)) {
+ debug(33, 3) ("ClientSocketContext::keepaliveNextRequest: half-closed client
with no pending requests, closing\n");
+ comm_close(conn->fd);
+ return;
+ }
+
ClientSocketContext::Pointer deferredRequest;
- if ((deferredRequest = conn->getCurrentContext()).getRaw() == NULL)
+ if ((deferredRequest = conn->getCurrentContext()).getRaw() == NULL) {
+ debug(33, 3) ("ClientSocketContext:: FD %d: calling
conn->readNextRequest()\n", conn->fd);
conn->readNextRequest();
- else
+ } else {
+ debug(33, 3) ("ClientSocketContext:: FD %d: calling PushDeferredIfNeeded\n",
conn->fd);
ClientSocketContextPushDeferredIfNeeded(deferredRequest, conn);
+ }
}
void
@@ -2275,15 +2299,85 @@
return result;
}
+/*
+ * Attempt to parse one or more requests from the input buffer.
+ * If a request is successfully parsed, even if the next request
+ * is only partially parsed, it will return TRUE.
+ * do_next_read is updated to indicate whether a read should be
+ * scheduled.
+ */
+static bool
+clientParseRequest(ConnStateData::Pointer conn, bool &do_next_read)
+{
+ method_t method;
+ char *prefix = NULL;
+ ClientSocketContext *context;
+ bool parsed_req = false;
+
+ while (conn->in.notYetUsed > 0 && conn->body.size_left == 0) {
+ size_t req_line_sz;
+ connStripBufferWhitespace (conn);
+
+ if (conn->in.notYetUsed == 0) {
+ clientAfterReadingRequests(conn->fd, conn, do_next_read);
+ break;
+ }
+
+ /* Limit the number of concurrent requests to 2 */
+ if (!connOkToAddRequest(conn)) {
+ break;
+ }
+
+ /* Should not be needed anymore */
+ /* Terminate the string */
+ conn->in.buf[conn->in.notYetUsed] = '\0';
+
+ /* Process request */
+ context = parseHttpRequest(conn, &method, &prefix, &req_line_sz);
+
+ /* partial or incomplete request */
+ if (!context) {
+ safe_free(prefix);
+
+ if (!connKeepReadingIncompleteRequest(conn))
+ connCancelIncompleteRequests(conn);
+
+ break;
+ }
+
+ /* status -1 or 1 */
+ if (context) {
+ commSetTimeout(conn->fd, Config.Timeout.lifetime, clientLifetimeTimeout,
+ context->http);
+
+ clientProcessRequest(conn, context, method, prefix, req_line_sz);
+
+ safe_free(prefix);
+ parsed_req = true;
+
+ if (context->mayUseConnection()) {
+ debug (33, 3) ("clientReadRequest: Not reading, as this request may
need the connection\n");
+ do_next_read = 0;
+ break;
+ }
+
+ if (!conn->flags.readMoreRequests) {
+ conn->flags.readMoreRequests = 1;
+ break;
+ }
+
+ continue; /* while offset > 0 && body.size_left == 0 */
+ }
+ } /* while offset > 0 && conn->body.size_left == 0 */
+ return parsed_req;
+}
+
static void
clientReadRequest(int fd, char *buf, size_t size, comm_err_t flag, int xerrno,
void *data)
{
ConnStateData::Pointer conn ((ConnStateData *)data);
conn->reading(false);
- method_t method;
- char *prefix = NULL;
- ClientSocketContext *context;
bool do_next_read = 1; /* the default _is_ to read data! - adrian */
assert (fd == conn->fd);
@@ -2347,68 +2441,21 @@
if (conn->getConcurrentRequestCount() == 0)
fd_note(conn->fd, "Reading next request");
- /* XXX: if we read *exactly* two requests, and the client sends no more,
- * if pipelined requests are off, we will *never* parse and insert the
- * second. the corner condition is due to the parsing being tied to the
- * read, not the presence of data in the buffer.
- */
- while (conn->in.notYetUsed > 0 && conn->body.size_left == 0) {
- size_t req_line_sz;
- connStripBufferWhitespace (conn);
-
- if (conn->in.notYetUsed == 0) {
- clientAfterReadingRequests(fd, conn, do_next_read);
- return;
- }
-
- /* Limit the number of concurrent requests to 2 */
- if (!connOkToAddRequest(conn)) {
- return;
- }
-
- /* Should not be needed anymore */
- /* Terminate the string */
- conn->in.buf[conn->in.notYetUsed] = '\0';
-
- /* Process request */
- context = parseHttpRequest(conn,
- &method, &prefix, &req_line_sz);
-
- /* partial or incomplete request */
- if (!context) {
- safe_free(prefix);
-
- if (!connKeepReadingIncompleteRequest(conn))
- connCancelIncompleteRequests(conn);
-
- break; /* conn->in.notYetUsed > 0 && conn->body.size_left == 0 */
- }
-
- /* status -1 or 1 */
- if (context) {
- commSetTimeout(fd, Config.Timeout.lifetime, clientLifetimeTimeout,
- context->http);
-
- clientProcessRequest(conn, context, method, prefix, req_line_sz);
-
- safe_free(prefix);
-
- if (context->mayUseConnection()) {
- debug (33, 3) ("clientReadRequest: Not reading, as this request may
need the connection\n");
- do_next_read = 0;
- break;
- }
-
- if (!conn->flags.readMoreRequests) {
- conn->flags.readMoreRequests = 1;
- break;
- }
-
- continue; /* while offset > 0 && body.size_left == 0 */
- }
- } /* while offset > 0 && conn->body.size_left == 0 */
-
- clientAfterReadingRequests(fd, conn, do_next_read);
+ if (clientParseRequest(conn, do_next_read))
+ clientAfterReadingRequests(fd, conn, do_next_read);
+ else {
+ /*
+ * If the client here is half closed and we failed
+ * to parse a request, close the connection.
+ * The above check with connFinishedWithConn() only
+ * succeeds _if_ the buffer is empty which it won't
+ * be if we have an incomplete request.
+ */
+ if (conn->getConcurrentRequestCount() == 0 && commIsHalfClosed(fd)) {
+ debug(33, 5) ("clientReadRequest: FD %d: half-closed connection, no
completed request parsed, connection closing.\n", fd);
+ comm_close(fd);
+ }
+ }
}
/* file_read like function, for reading body content */
Index: comm.cc
===================================================================
RCS file: /server/cvs-server/squid/squid3/src/comm.cc,v
retrieving revision 1.392
diff -u -r1.392 comm.cc
--- comm.cc 18 Feb 2004 01:58:59 -0000 1.392
+++ comm.cc 26 Feb 2004 06:47:24 -0000
@@ -2560,6 +2560,21 @@
fdc_table[fd].half_closed = true;
}
+int commIsHalfClosed(int fd)
+{
+ if (fdc_table[fd].active != 1) {
+ fatal("foo");
+ }
+ return fdc_table[fd].half_closed;
+}
+
+void
+commCheckHalfClosed(void *data)
+{
+ AbortChecker::Instance().doIOLoop();
+ eventAdd("commCheckHalfClosed", commCheckHalfClosed, NULL, 1.0, false);
+}
+
AbortChecker &AbortChecker::Instance() {return Instance_;}
AbortChecker AbortChecker::Instance_;
@@ -2601,22 +2616,8 @@
#include "splay.h"
void
AbortChecker::doIOLoop() {
- if (checking) {
- /*
- fds->walk(RemoveCheck, this);
- */
- checking = false;
- return;
- }
-
- if (lastCheck >= squid_curtime)
- return;
-
+ fds->walk(RemoveCheck, this);
fds->walk(AddCheck, this);
-
- checking = true;
-
- lastCheck = squid_curtime;
}
void
Index: main.cc
===================================================================
RCS file: /server/cvs-server/squid/squid3/src/main.cc,v
retrieving revision 1.389
diff -u -r1.389 main.cc
--- main.cc 19 Sep 2003 07:06:19 -0000 1.389
+++ main.cc 26 Feb 2004 06:47:24 -0000
@@ -44,6 +44,7 @@
#include "ACL.h"
#include "htcp.h"
#include "StoreFileSystem.h"
+#include "comm.h"
#if USE_WIN32_SERVICE
@@ -837,6 +838,7 @@
#endif
eventAdd("memPoolCleanIdlePools", Mem::CleanIdlePools, NULL, 15.0, 1);
+ eventAdd("commCheckHalfClosed", commCheckHalfClosed, NULL, 1.0, false);
}
configured_once = 1;
Index: comm.h
===================================================================
RCS file: /server/cvs-server/squid/squid3/src/comm.h,v
retrieving revision 1.20
diff -u -r1.20 comm.h
--- comm.h 15 Aug 2003 13:06:34 -0000 1.20
+++ comm.h 26 Feb 2004 06:47:24 -0000
@@ -30,6 +30,8 @@
extern void comm_write(int s, const char *buf, size_t len, IOWCB *callback, void
*callback_data);
#include "Store.h"
extern void commMarkHalfClosed(int);
+extern int commIsHalfClosed(int);
+extern void commCheckHalfClosed(void *);
extern bool comm_has_incomplete_write(int);
/* Where should this belong? */
