From: wessels <> Date: Sat, 6 May 2006 03:33:56 +0000 (+0000) Subject: Fix for : assertion failed: forward.cc:99: "err" X-Git-Tag: SQUID_3_0_PRE4~197 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=b2d22df64d7e307b68afa3bc3adca70bf202b0b3;p=thirdparty%2Fsquid.git Fix for : assertion failed: forward.cc:99: "err" This assertion was triggered when FwdState tries to re-forward a request. The first request fails, but can be retried. If the first ServerState gets destroyed before the second ServerState gets created, the FwdState refcount goes to zero. The FwdState destructor has the above assertion because it expects an ErrorState for the StoreEntry with no content. This condition can easily happen because the second ServerState isn't created until the origin server connection is established. ie: http.cc(1318) transactionComplete FD -1 this 0x8a430b0 fwdComplete: re-forwarding 504 http://www.example.org/ fwdConnectStart ... commConnectHandle: ...: COMM_INPROGRESS httpStateFree: ... forward.cc(87) FwdState destructor starting assertion failed: forward.cc:99: "err" The clientReplyContext class now keeps a refcounted pointer to FwdState. This was done so that FwdState refcount doesn't go to zero during a "reforward" between the time when the initial ServerSide gets destroyed and before the second attempt ServerSide gets created. We don't really want to keep FwdState around until the client is finished. As an future optimization we can remove the client side reference around the time when the ENTRY_FWD_HDR_WAIT bit gets cleared. --- diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index f54da21685..3ec12c2eb9 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1,6 +1,6 @@ /* - * $Id: client_side_reply.cc,v 1.100 2006/04/27 16:54:16 wessels Exp $ + * $Id: client_side_reply.cc,v 1.101 2006/05/05 21:33:56 wessels Exp $ * * DEBUG: section 88 Client-side Reply Routines * AUTHOR: Robert Collins (Originally Duane Wessels in client_side.c) @@ -80,6 +80,7 @@ clientReplyContext::~clientReplyContext() safe_free(tempBuffer.data); cbdataReferenceDone(http); HTTPMSGUNLOCK(reply); + fwd = NULL; // refcounted } clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) : http (cbdataReference(clientContext)), old_entry (NULL), old_sc(NULL), deleting(false) @@ -267,9 +268,14 @@ clientReplyContext::processExpired() (long int) entry->lastmod); http->storeEntry(entry); assert(http->out.offset == 0); - FwdState::fwdStart(http->getConn().getRaw() != NULL ? http->getConn()->fd : -1, - http->storeEntry(), - http->request); + + /* + * A refcounted pointer so that FwdState stays around as long as + * this clientReplyContext does + */ + fwd = FwdState::fwdStart(http->getConn().getRaw() != NULL ? http->getConn()->fd : -1, + http->storeEntry(), + http->request); /* Register with storage manager to receive updates when data comes in. */ if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) diff --git a/src/client_side_reply.h b/src/client_side_reply.h index bb9e604acb..dda25ae70d 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -1,6 +1,6 @@ /* - * $Id: client_side_reply.h,v 1.10 2006/02/18 00:23:43 wessels Exp $ + * $Id: client_side_reply.h,v 1.11 2006/05/05 21:33:56 wessels Exp $ * * * SQUID Web Proxy Cache http://www.squid-cache.org/ @@ -41,6 +41,15 @@ #include "StoreClient.h" #include "client_side_request.h" +/* + * The clientReplyContext class now keeps a refcounted pointer to + * FwdState. This was done so that FwdState refcount doesn't go + * to zero during a "reforward" between the time when the initial + * ServerSide gets destroyed and before the second attempt ServerSide + * gets created. + */ +#include "forward.h" + /* XXX make static method */ class clientReplyContext : public RefCountable, public StoreClient @@ -129,6 +138,7 @@ private: void startSendProcess(); StoreIOBuffer holdingBuffer; HttpReply *reply; + FwdState::Pointer fwd; void processReplyAccess(); static PF ProcessReplyAccessResult; void processReplyAccessResult(bool accessAllowed); diff --git a/src/forward.cc b/src/forward.cc index 598f2259c6..dc420957f7 100644 --- a/src/forward.cc +++ b/src/forward.cc @@ -1,6 +1,6 @@ /* - * $Id: forward.cc,v 1.138 2006/04/27 19:27:37 wessels Exp $ + * $Id: forward.cc,v 1.139 2006/05/05 21:33:56 wessels Exp $ * * DEBUG: section 17 Request Forwarding * AUTHOR: Duane Wessels @@ -137,7 +137,7 @@ FwdState::~FwdState() * a transaction. It is a static method that may or may not * allocate a FwdState. */ -void +FwdState * FwdState::fwdStart(int client_fd, StoreEntry *entry, HttpRequest *request) { /* @@ -174,7 +174,7 @@ FwdState::fwdStart(int client_fd, StoreEntry *entry, HttpRequest *request) errorAppendEntry(entry, anErr); // frees anErr - return; + return NULL; } } @@ -194,27 +194,27 @@ FwdState::fwdStart(int client_fd, StoreEntry *entry, HttpRequest *request) ErrorState *anErr = errorCon(ERR_SHUTTING_DOWN, HTTP_SERVICE_UNAVAILABLE); anErr->request = HTTPMSGLOCK(request); errorAppendEntry(entry, anErr); // frees anErr - return; + return NULL; } switch (request->protocol) { case PROTO_INTERNAL: internalStart(request, entry); - return; + return NULL; case PROTO_CACHEOBJ: cachemgrStart(client_fd, request, entry); - return; + return NULL; case PROTO_URN: urnStart(request, entry); - return; + return NULL; default: FwdState *fwd = new FwdState(client_fd, entry, request); peerSelect(request, entry, fwdStartCompleteWrapper, fwd); - return; + return fwd; } /* NOTREACHED */ diff --git a/src/forward.h b/src/forward.h index 12866d0296..44a806484e 100644 --- a/src/forward.h +++ b/src/forward.h @@ -19,7 +19,7 @@ public: ~FwdState(); static void initModule(); - static void fwdStart(int fd, StoreEntry *, HttpRequest *); + static FwdState * fwdStart(int fd, StoreEntry *, HttpRequest *); void startComplete(FwdServer *); void startFail(); void fail(ErrorState *err);