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 f54da216851136957646acefb5adc9e234699064..3ec12c2eb9b818cbfa62aaa9d8fdc935f745758f 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.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 bb9e604acb5f7aee84a31cb990c9c7fe11051633..dda25ae70d5350a43144fc94e22bd6fdc5ba498a 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.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 598f2259c6e4dacef09b6e69594a4033e0da6cf9..dc420957f7a454eaf0970cf8b2a571c614f89fa1 100644 (file)
--- 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 12866d02967848a330daf13195e1ee85dad1592b..44a806484edf780f6b1f0a82f3c2f21db783812d 100644 (file)
--- 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);