From: robertc <> Date: Sun, 6 Apr 2003 14:23:10 +0000 (+0000) Subject: Summary: Fix POST. X-Git-Tag: SQUID_3_0_PRE1~253 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=50c09fc434a5318fa2a7b7d7045dddc1d1628ebb;p=thirdparty%2Fsquid.git 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 a6eb930d49..3dcd0a947d 100644 --- 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 7694e1dfbc..56166776df 100644 --- 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(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 66850898fa..14ab1db8b0 100644 --- 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 43d4a40d42..cc610e0061 100644 --- 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 */