From: wessels <> Date: Sat, 18 Feb 2006 07:23:43 +0000 (+0000) Subject: Cleaned up HttpReply handling in client_side_reply code. X-Git-Tag: SQUID_3_0_PRE4~327 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7dc5f514b9b374ed72b17d2c4f3662bd1e464966;p=thirdparty%2Fsquid.git Cleaned up HttpReply handling in client_side_reply code. ACLChecklist should lock/unlock the HttpReply. clientReplyContext had a "holdingReply" member which I had a hard time figuring out. My guess is that it was just a way to avoid leaking the HttpReply. I've replaced it with a more permanent HttpReply which is now locked and unlocked as necessary. --- diff --git a/src/ACLChecklist.cc b/src/ACLChecklist.cc index 6554058c16..940c41ecbc 100644 --- a/src/ACLChecklist.cc +++ b/src/ACLChecklist.cc @@ -1,5 +1,5 @@ /* - * $Id: ACLChecklist.cc,v 1.30 2006/02/17 18:10:59 wessels Exp $ + * $Id: ACLChecklist.cc,v 1.31 2006/02/18 00:23:43 wessels Exp $ * * DEBUG: section 28 Access Control * AUTHOR: Duane Wessels @@ -36,6 +36,7 @@ #include "squid.h" #include "ACLChecklist.h" #include "HttpRequest.h" +#include "HttpReply.h" #include "authenticate.h" #include "ACLProxyAuth.h" #include "client_side.h" @@ -325,6 +326,8 @@ ACLChecklist::~ACLChecklist() HTTPMSGUNLOCK(request); + HTTPMSGUNLOCK(reply); + conn_ = NULL; cbdataReferenceDone(accessList); diff --git a/src/client_side.cc b/src/client_side.cc index 1307c9fef9..b80c2f682d 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side.cc,v 1.712 2006/02/18 00:04:30 wessels Exp $ + * $Id: client_side.cc,v 1.713 2006/02/18 00:23:43 wessels Exp $ * * DEBUG: section 33 Client-side Routines * AUTHOR: Duane Wessels @@ -522,7 +522,7 @@ ClientHttpRequest::logRequest() ACLChecklist *checklist = clientAclChecklistCreate(Config.accessList.log, this); - checklist->reply = al.reply; + checklist->reply = HTTPMSGLOCK(al.reply); if (!Config.accessList.log || checklist->fastCheck()) { al.request = HTTPMSGLOCK(request); @@ -1216,8 +1216,9 @@ ClientSocketContext::sendStartOfMessage(HttpReply * rep, StoreIOBuffer bodyData) } /* - * Write a chunk of data to a client socket. If the reply is present, send the reply headers down the wire too, - * and clean them up when finished. + * Write a chunk of data to a client socket. If the reply is present, + * send the reply headers down the wire too, and clean them up when + * finished. * Pre-condition: * The request is one backed by a connection, not an internal request. * data context is not NULL diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index fc594f7ae2..11f8c5cc74 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side_reply.cc,v 1.96 2006/02/17 20:15:35 wessels Exp $ + * $Id: client_side_reply.cc,v 1.97 2006/02/18 00:23:43 wessels Exp $ * * DEBUG: section 88 Client-side Reply Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -991,8 +991,6 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) purgeStatus = HTTP_OK; } - HttpReply *r; - /* And for Vary, release the base URI if none of the headers was included in the request */ if (http->request->vary_headers @@ -1027,13 +1025,13 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) triggerInitialStoreRead(); - r = new HttpReply; + HttpReply *rep = new HttpReply; HttpVersion version(1,0); - r->setHeaders(version, purgeStatus, NULL, NULL, 0, 0, -1); + rep->setHeaders(version, purgeStatus, NULL, NULL, 0, 0, -1); - http->storeEntry()->replaceHttpReply(r); + http->storeEntry()->replaceHttpReply(rep); http->storeEntry()->complete(); } @@ -1041,7 +1039,6 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) void clientReplyContext::traceReply(clientStreamNode * node) { - HttpReply *rep; clientStreamNode *next = (clientStreamNode *)node->node.next->data; StoreIOBuffer tempBuffer; assert(http->request->max_forwards == 0); @@ -1053,7 +1050,7 @@ clientReplyContext::traceReply(clientStreamNode * node) tempBuffer, SendMoreData, this); storeReleaseRequest(http->storeEntry()); storeBuffer(http->storeEntry()); - rep = new HttpReply; + HttpReply *rep = new HttpReply; HttpVersion version(1,0); rep->setHeaders(version, HTTP_OK, NULL, "text/plain", http->request->prefixLen(), 0, squid_curtime); @@ -1286,13 +1283,6 @@ clientReplyContext::alwaysAllowResponse(http_status sline) const return result; } -void -clientReplyContext::obeyConnectionHeader() -{ - HttpHeader *hdr = &holdingReply->header; - hdr->removeConnectionHeaderEntries(); -} - /* * filters out unwanted entries from original reply header * adds extra entries if we have more info than origin server @@ -1301,7 +1291,7 @@ clientReplyContext::obeyConnectionHeader() void clientReplyContext::buildReplyHeader() { - HttpHeader *hdr = &holdingReply->header; + HttpHeader *hdr = &reply->header; int is_hit = logTypeIsATcpHit(http->logType); HttpRequest *request = http->request; #if DONT_FILTER_THESE @@ -1318,10 +1308,13 @@ clientReplyContext::buildReplyHeader() if (is_hit) httpHeaderDelById(hdr, HDR_SET_COOKIE); - obeyConnectionHeader(); + /* + * Be sure to obey the Connection header + */ + reply->header.removeConnectionHeaderEntries(); // if (request->range) - // clientBuildRangeHeader(http, holdingReply); + // clientBuildRangeHeader(http, reply); /* * Add a estimated Age header on cache hits. */ @@ -1397,7 +1390,7 @@ clientReplyContext::buildReplyHeader() /* Handle authentication headers */ if (request->auth_user_request) - authenticateFixHeader(holdingReply, request->auth_user_request, request, + authenticateFixHeader(reply, request->auth_user_request, request, http->flags.accel, 0); /* Append X-Cache */ @@ -1412,7 +1405,7 @@ clientReplyContext::buildReplyHeader() #endif - if (holdingReply->bodySize(request->method) < 0) { + if (reply->bodySize(request->method) < 0) { debug(88, 3) ("clientBuildReplyHeader: can't keep-alive, unknown body size\n"); @@ -1432,8 +1425,8 @@ clientReplyContext::buildReplyHeader() LOCAL_ARRAY(char, bbuf, MAX_URL + 32); String strVia = httpHeaderGetList(hdr, HDR_VIA); snprintf(bbuf, sizeof(bbuf), "%d.%d %s", - holdingReply->sline.version.major, - holdingReply->sline.version.minor, + reply->sline.version.major, + reply->sline.version.minor, ThisCache); strListAdd(&strVia, bbuf, ','); httpHeaderDelById(hdr, HDR_VIA); @@ -1469,26 +1462,27 @@ clientReplyContext::buildReply(const char *buf, size_t size) if (!k) return; - holdReply(new HttpReply); + assert(reply == NULL); - if (!holdingReply->parseCharBuf(buf, k)) { + HttpReply *rep = new HttpReply; + + reply = HTTPMSGLOCK(rep); + + if (!reply->parseCharBuf(buf, k)) { /* parsing failure, get rid of the invalid reply */ - delete holdingReply; - holdReply (NULL); - /* This is wrong. ~HttpReply() should to the rep - * for us, and we can destroy our own range info - */ + HTTPMSGUNLOCK(reply); if (http->request->range) { + debugs(0,0,HERE << "look for bug here"); /* this will fail and destroy request->range */ - // clientBuildRangeHeader(http, holdingReply); + // clientBuildRangeHeader(http, reply); } return; } /* enforce 1.0 reply version */ - holdingReply->sline.version = HttpVersion(1,0); + reply->sline.version = HttpVersion(1,0); /* do header conversions */ buildReplyHeader(); @@ -1817,13 +1811,6 @@ clientReplyContext::startSendProcess() tempBuffer, SendMoreData, this); } -void -clientReplyContext::holdReply(HttpReply *aReply) -{ - assert (!holdingReply || !aReply); - holdingReply = aReply; -} - /* * Calculates the maximum size allowed for an HTTP response */ @@ -1838,7 +1825,7 @@ clientReplyContext::buildMaxBodySize(HttpReply * reply) ch = clientAclChecklistCreate(NULL, http); - ch->reply = reply; + ch->reply = HTTPMSGLOCK(reply); for (l = Config.ReplyBodySize; l; l = l -> next) { if (ch->matchAclListFast(l->aclList)) { @@ -1857,26 +1844,24 @@ clientReplyContext::buildMaxBodySize(HttpReply * reply) void clientReplyContext::processReplyAccess () { - HttpReply *rep = holdingReply; - holdReply(NULL); - buildMaxBodySize(rep); + assert(reply); + buildMaxBodySize(reply); - if (http->isReplyBodyTooLarge(rep->content_length)) { + if (http->isReplyBodyTooLarge(reply->content_length)) { ErrorState *err = clientBuildError(ERR_TOO_BIG, HTTP_FORBIDDEN, NULL, http->getConn().getRaw() != NULL ? &http->getConn()->peer.sin_addr : &no_addr, http->request); removeClientStoreReference(&sc, http); startError(err); - delete rep; + HTTPMSGUNLOCK(reply); return; } - headers_sz = rep->hdr_sz; + headers_sz = reply->hdr_sz; ACLChecklist *replyChecklist; replyChecklist = clientAclChecklistCreate(Config.accessList.reply, http); - replyChecklist->reply = rep; - holdReply (rep); + replyChecklist->reply = HTTPMSGLOCK(reply); replyChecklist->nonBlockingCheck(ProcessReplyAccessResult, this); } @@ -1894,11 +1879,9 @@ clientReplyContext::processReplyAccessResult(bool accessAllowed) RequestMethodStr[http->request->method], http->uri, accessAllowed ? "ALLOWED" : "DENIED", AclMatchedName ? AclMatchedName : "NO ACL's"); - HttpReply *rep = holdingReply; - holdReply (NULL); - if (!accessAllowed && rep->sline.status != HTTP_FORBIDDEN - && !alwaysAllowResponse(rep->sline.status)) { + if (!accessAllowed && reply->sline.status != HTTP_FORBIDDEN + && !alwaysAllowResponse(reply->sline.status)) { /* the if above is slightly broken, but there is no way * to tell if this is a squid generated error page, or one from * upstream at this point. */ @@ -1918,7 +1901,7 @@ clientReplyContext::processReplyAccessResult(bool accessAllowed) startError(err); - delete rep; + HTTPMSGUNLOCK(reply); http->logType = LOG_TCP_DENIED_REPLY; @@ -1928,19 +1911,19 @@ clientReplyContext::processReplyAccessResult(bool accessAllowed) /* Ok, the reply is allowed, */ http->loggingEntry(http->storeEntry()); - ssize_t body_size = reqofs - rep->hdr_sz; + ssize_t body_size = reqofs - reply->hdr_sz; assert(body_size >= 0); debug(88,3) ("clientReplyContext::sendMoreData: Appending %d bytes after %d bytes of headers\n", - (int) body_size, rep->hdr_sz); + (int) body_size, reply->hdr_sz); #if ESI - if (http->flags.accel && rep->sline.status != HTTP_FORBIDDEN && - !alwaysAllowResponse(rep->sline.status) && - esiEnableProcessing(rep)) { + if (http->flags.accel && reply->sline.status != HTTP_FORBIDDEN && + !alwaysAllowResponse(reply->sline.status) && + esiEnableProcessing(reply)) { debug(88, 2) ("Enabling ESI processing for %s\n", http->uri); clientStreamInsertHead(&http->client_stream, esiStreamRead, esiProcessStream, esiStreamDetach, esiStreamStatus, NULL); @@ -1972,7 +1955,7 @@ clientReplyContext::processReplyAccessResult(bool accessAllowed) StoreIOBuffer tempBuffer; char *buf = next()->readBuffer.data; - char *body_buf = buf + rep->hdr_sz; + char *body_buf = buf + reply->hdr_sz; //Server side may disable ranges under some circumstances. @@ -1995,7 +1978,7 @@ clientReplyContext::processReplyAccessResult(bool accessAllowed) /* TODO??: move the data in the buffer back by the request header size */ clientStreamCallback((clientStreamNode *)http->client_stream.head->data, - http, rep, tempBuffer); + http, reply, tempBuffer); return; } @@ -2081,7 +2064,7 @@ clientReplyContext::sendMoreData (StoreIOBuffer result) buildReply(buf, reqofs); ssize_t body_size = reqofs; - if (holdingReply) { + if (reply) { /* handle headers */ @@ -2096,7 +2079,7 @@ clientReplyContext::sendMoreData (StoreIOBuffer result) } holdingBuffer = result; - processReplyAccess (); + processReplyAccess(); return; } else if (reqofs < HTTP_REQBUF_SZ && entry->store_status == STORE_PENDING) { diff --git a/src/client_side_reply.h b/src/client_side_reply.h index 1c26194c81..bb9e604acb 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -1,6 +1,6 @@ /* - * $Id: client_side_reply.h,v 1.9 2005/12/01 21:35:40 serassio Exp $ + * $Id: client_side_reply.h,v 1.10 2006/02/18 00:23:43 wessels Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -128,15 +128,13 @@ private: clientStreamNode * next() const; void startSendProcess(); StoreIOBuffer holdingBuffer; - HttpReply *holdingReply; + HttpReply *reply; void processReplyAccess(); static PF ProcessReplyAccessResult; void processReplyAccessResult(bool accessAllowed); void buildReply(const char *buf, size_t size); void buildReplyHeader (); - void holdReply(HttpReply *); bool alwaysAllowResponse(http_status sline) const; - void obeyConnectionHeader(); int checkTransferDone(); void processOnlyIfCachedMiss(); void cacheHit(StoreIOBuffer result);