Summary: Fix POST.

Keywords:

* Fix debug statements in client_side that where moved out of clientReadRequest to refer to their new function name.
* Refactor clientProcessBody into somewhat smaller chunks.
* Fix hung connections where more data was not retrieved in large POSTS.

Summary: Fix race when deleting clientReplyContext's.
Keywords:

Fix race when deleting clientReplyContext's. clientReplyContext::operator delete will cancel store reads and may trigger callbacks. However the object is no longer ready to handle those...

diff --git a/src/clientStream.cc b/src/clientStream.cc
index a6eb930d49bace43f9f4c54b09f6e322d0d2655c..3dcd0a947de63652a5154149d56545d6cc4e1f3c 100644 (file)
--- a/src/clientStream.cc
+++ b/src/clientStream.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: clientStream.cc,v 1.7 2003/03/15 04:17:39 robertc Exp $
+ * $Id: clientStream.cc,v 1.8 2003/04/06 08:23:10 robertc Exp $
  *
  * DEBUG: section 87    Client-side Stream routines.
  * AUTHOR: Robert Collins
@@ -97,7 +97,7 @@ CBDATA_TYPE(clientStreamNode);
  * (i.e. 
  * mycontext = thisObject->data;
  * thisObject->data = NULL;
- * clientStreamFreeLinst (thisObject->head);
+ * clientStreamFree (thisObject->head);
  * mycontext = NULL;
  * return;
  */

diff --git a/src/client_side.cc b/src/client_side.cc
index 7694e1dfbc69dc5775062adc331cd4c0badbd6c3..56166776df92913de427071856ccf68841cbecca 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side.cc,v 1.634 2003/03/15 04:17:39 robertc Exp $
+ * $Id: client_side.cc,v 1.635 2003/04/06 08:23:10 robertc Exp $
  *
  * DEBUG: section 33    Client-side Routines
  * AUTHOR: Duane Wessels
@@ -142,7 +142,25 @@ static int clientIsContentLengthValid(request_t * r);
 static bool okToAccept();
 static int clientIsRequestBodyValid(int bodyLength);
 static int clientIsRequestBodyTooLargeForPolicy(size_t bodyLength);
-static void clientProcessBody(ConnStateData * conn);
+/* convenience class while splitting up body handling */
+/* temporary existence only - on stack use expected */
+
+class ClientBody
+{
+
+public:
+    ClientBody (ConnStateData *);
+    void process();
+    void preProcessing();
+    void processBuffer();
+
+private:
+    ConnStateData *conn;
+    char *buf;
+    CBCB *callback;
+    request_t *request;
+};
+
 static void clientUpdateStatHistCounters(log_type logType, int svc_time);
 static void clientUpdateStatCounters(log_type logType);
 static void clientUpdateHierCounters(HierarchyLogEntry *);
@@ -2032,7 +2050,7 @@ clientAfterReadingRequests(int fd, ConnStateData *conn, int do_next_read)
         if (conn->in.notYetUsed != conn->body.size_left) {
             /* != 0 when no request body */
             /* Partial request received. Abort client connection! */
-            debug(33, 3) ("clientReadRequest: FD %d aborted, partial request\n",+                         fd);
+            debug(33, 3) ("clientAfterReadingRequests: FD %d aborted, partial request\n",+                         fd);
             comm_close(fd);
             return;
         }
@@ -2054,7 +2072,7 @@ clientProcessRequest(ConnStateData *conn, ClientSocketContext *context, method_t
 
     if (context->flags.parsed_ok == 0) {
         clientStreamNode *node = context->getClientReplyContext();
-        debug(33, 1) ("clientReadRequest: Invalid Request\n");
+        debug(33, 1) ("clientProcessRequest: Invalid Request\n");
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
         assert (repContext);
         repContext->setReplyToError(ERR_INVALID_REQ, HTTP_BAD_REQUEST, method, NULL,
@@ -2189,9 +2207,9 @@ connOkToAddRequest(ConnStateData *conn)
     int result = conn->getConcurrentRequestCount() < (Config.onoff.pipeline_prefetch ? 2 : 1);
 
     if (!result) {
-        debug(33, 3) ("clientReadRequest: FD %d max concurrent requests reached\n",
+        debug(33, 3) ("connOkToAddRequest: FD %d max concurrent requests reached\n",
                       conn->fd);
-        debug(33, 5) ("clientReadRequest: FD %d defering new request until one is done\n",
+        debug(33, 5) ("connOkToAddRequest: FD %d defering new request until one is done\n",
                       conn->fd);
     }
 
@@ -2261,8 +2279,10 @@ clientReadRequest(int fd, char *buf, size_t size, comm_err_t flag, int xerrno,
 
 
     /* Process request body if any */
-    if (conn->in.notYetUsed > 0 && conn->body.callback != NULL)
-        clientProcessBody(conn);
+    if (conn->in.notYetUsed > 0 && conn->body.callback != NULL) {
+        ClientBody body(conn);
+        body.process();
+    }
 
     /* Process next request */
     if (conn->getConcurrentRequestCount() == 0)
@@ -2353,79 +2373,96 @@ clientReadBody(request_t * request, char *buf, size_t size, CBCB * callback,
     conn->body.buf = buf;
     conn->body.bufsize = size;
     conn->body.request = requestLink(request);
-    clientProcessBody(conn);
+    ClientBody body (conn);
+    body.process();
 }
 
-/* Called by clientReadRequest to process body content */
-static void
-clientProcessBody(ConnStateData * conn)
+ClientBody::ClientBody(ConnStateData *aConn) : conn(aConn), buf (NULL), callback(NULL), request(NULL)
+{}
+
+void
+ClientBody::preProcessing()
 {
-    size_t size;
-    char *buf = conn->body.buf;
-    void *cbdata = conn->body.cbdata;
-    CBCB *callback = conn->body.callback;
-    request_t *request = conn->body.request;
+    callback = conn->body.callback;
+    request = conn->body.request;
     /* Note: request is null while eating "aborted" transfers */
-    debug(33, 2) ("clientProcessBody: start fd=%d body_size=%lu in.notYetUsed=%lu cb=%p req=%p\n",
+    debug(33, 2) ("clientBody::process: start fd=%d body_size=%lu in.notYetUsed=%lu cb=%p req=%p\n",
                   conn->fd, (unsigned long int) conn->body.size_left,
                   (unsigned long int) conn->in.notYetUsed, callback, request);
+}
 
-    if (conn->in.notYetUsed) {
-        /* Some sanity checks... */
-        assert(conn->body.size_left > 0);
-        assert(conn->in.notYetUsed > 0);
-        assert(callback != NULL);
-        assert(buf != NULL);
-        /* How much do we have to process? */
-        size = conn->in.notYetUsed;
+/* Called by clientReadRequest to process body content */
+void
+ClientBody::process()
+{
+    preProcessing();
 
-        if (size > conn->body.size_left)       /* only process the body part */
-            size = conn->body.size_left;
+    if (conn->in.notYetUsed)
+        processBuffer();
+    else
+        conn->readSomeData();
+}
 
-        if (size > conn->body.bufsize) /* don't copy more than requested */
-            size = conn->body.bufsize;
+void
+ClientBody::processBuffer()
+{
+    /* Some sanity checks... */
+    assert(conn->body.size_left > 0);
+    assert(conn->in.notYetUsed > 0);
+    assert(callback != NULL);
+    buf = conn->body.buf;
+    assert(buf != NULL);
+    /* How much do we have to process? */
+    size_t size = conn->in.notYetUsed;
 
-        xmemcpy(buf, conn->in.buf, size);
+    if (size > conn->body.size_left)   /* only process the body part */
+        size = conn->body.size_left;
 
-        conn->body.size_left -= size;
+    if (size > conn->body.bufsize)     /* don't copy more than requested */
+        size = conn->body.bufsize;
 
-        /* Move any remaining data */
-        conn->in.notYetUsed -= size;
+    xmemcpy(buf, conn->in.buf, size);
 
-        if (conn->in.notYetUsed > 0)
-            xmemmove(conn->in.buf, conn->in.buf + size, conn->in.notYetUsed);
+    conn->body.size_left -= size;
 
-        /* Remove request link if this is the last part of the body, as
-         * clientReadRequest automatically continues to process next request */
-        if (conn->body.size_left <= 0 && request != NULL)
-            request->body_connection = NULL;
+    /* Move any remaining data */
+    conn->in.notYetUsed -= size;
 
-        /* Remove clientReadBody arguments (the call is completed) */
-        conn->body.request = NULL;
+    if (conn->in.notYetUsed > 0)
+        xmemmove(conn->in.buf, conn->in.buf + size, conn->in.notYetUsed);
 
-        conn->body.callback = NULL;
+    /* Remove request link if this is the last part of the body, as
+     * clientReadRequest automatically continues to process next request */
+    if (conn->body.size_left <= 0 && request != NULL)
+        request->body_connection = NULL;
 
-        conn->body.buf = NULL;
+    /* Remove clientReadBody arguments (the call is completed) */
+    conn->body.request = NULL;
 
-        conn->body.bufsize = 0;
+    conn->body.callback = NULL;
 
-        /* Remember that we have touched the body, not restartable */
-        if (request != NULL) {
-            request->flags.body_sent = 1;
-            conn->body.request = NULL;
-        }
+    conn->body.buf = NULL;
 
-        /* Invoke callback function */
-        callback(buf, size, cbdata);
+    conn->body.bufsize = 0;
 
-        if (request != NULL) {
-            requestUnlink(request);    /* Linked in clientReadBody */
-        }
+    /* Remember that we have touched the body, not restartable */
+    if (request != NULL) {
+        request->flags.body_sent = 1;
+        conn->body.request = NULL;
+    }
 
-        debug(33, 2) ("clientProcessBody: end fd=%d size=%lu body_size=%lu in.notYetUsed=%lu cb=%p req=%p\n",
-                      conn->fd, (unsigned long int)size, (unsigned long int) conn->body.size_left,
-                      (unsigned long) conn->in.notYetUsed, callback, request);
+    /* Invoke callback function */
+    void *cbdata = conn->body.cbdata;
+
+    callback(buf, size, cbdata);
+
+    if (request != NULL) {
+        requestUnlink(request);        /* Linked in clientReadBody */
     }
+
+    debug(33, 2) ("ClientBody::process: end fd=%d size=%lu body_size=%lu in.notYetUsed=%lu cb=%p req=%p\n",
+                  conn->fd, (unsigned long int)size, (unsigned long int) conn->body.size_left,
+                  (unsigned long) conn->in.notYetUsed, callback, request);
 }
 
 /* A dummy handler that throws away a request-body */
@@ -2475,7 +2512,7 @@ clientAbortBody(request_t * request)
     }
 
     clientReadBodyAbortHandler(NULL, -1, conn);        /* Install abort handler */
-    /* clientProcessBody() */
+    /* ClientBody::process() */
     return 1;                  /* Aborted */
 }
 

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 66850898faa20a5cb5d548b7e61bac47f759d550..14ab1db8b00e9c48e2057d41145cbbd93b13d308 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_reply.cc,v 1.47 2003/03/15 04:17:39 robertc Exp $
+ * $Id: client_side_reply.cc,v 1.48 2003/04/06 08:23:10 robertc Exp $
  *
  * DEBUG: section 88    Client-side Reply Routines
  * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c)
@@ -65,6 +65,10 @@ extern ErrorState *clientBuildError(err_type, http_status, char const *,
 
 clientReplyContext::~clientReplyContext()
 {
+    deleting = true;
+    /* This may trigger a callback back into SendMoreData as the cbdata
+     * is still valid
+     */
     removeStoreReference(&sc, &http->entry);
     /* old_entry might still be set if we didn't yet get the reply
      * code in HandleIMSReply() */
@@ -73,7 +77,7 @@ clientReplyContext::~clientReplyContext()
     cbdataReferenceDone(http);
 }
 
-clientReplyContext::clientReplyContext(clientHttpRequest *clientContext) : http (cbdataReference(clientContext)), old_entry (NULL), old_sc(NULL)
+clientReplyContext::clientReplyContext(clientHttpRequest *clientContext) : http (cbdataReference(clientContext)), old_entry (NULL), old_sc(NULL), deleting(false)
 {}
 
 /* create an error in the store awaiting the client side to read it. */
@@ -1913,10 +1917,17 @@ clientReplyContext::processReplyAccessResult(bool accessAllowed)
 void
 clientReplyContext::sendMoreData (StoreIOBuffer result)
 {
+    if (deleting)
+        return;
+
     StoreEntry *entry = http->entry;
+
     ConnStateData *conn = http->conn;
+
     int fd = conn ? conn->fd : -1;
+
     char *buf = next()->readBuffer.data;
+
     char *body_buf = buf;
 
     /* This is always valid until we get the headers as metadata from

diff --git a/src/client_side_reply.h b/src/client_side_reply.h
index 43d4a40d4226b76d9433c5ae92288f1450561000..cc610e006189c8eca696d7cd112bed00b1b58e4c 100644 (file)
--- a/src/client_side_reply.h
+++ b/src/client_side_reply.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: client_side_reply.h,v 1.1 2003/03/15 04:27:27 robertc Exp $
+ * $Id: client_side_reply.h,v 1.2 2003/04/06 08:23:10 robertc Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -151,6 +151,7 @@ private:
 
     StoreEntry *old_entry;
     store_client *old_sc;      /* ... for entry to be validated */
+    bool deleting;
 };
 
 #endif /* SQUID_CLIENTSIDEREPLY_H */