From: rousskov <>
Date: Thu, 12 Apr 2007 20:07:10 +0000 (+0000)
Subject: This change should fix bug #1637.
X-Git-Tag: SQUID_3_0_PRE6~114
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7a0fb323e9b5d74e421fa60fc8ce07c081fc44c5;p=thirdparty%2Fsquid.git

This change should fix bug #1637.

Server transactions keep refcounted pointer to FwdState that launched them.
Server transactions use the pointer to call FwdState upon completion or
abnormal termination. If nobody else points to the FwdState object after the
transaction is done, the object gets destroyed, even if there are other
servers to be tried.

To avoid FwdState destruction when selecting the next server, FwdState must
keep a refcounted pointer to self. That pointer should be created before the
server-selection loop and cleared after the loop. Currently, we create the
pointer when FwdState is created (which is earlier than needed) and clear it
in various places (which may be late and is probably buggy).

Based on Christos Tsantilas' work.
---

diff --git a/src/forward.cc b/src/forward.cc
index 7d114230e7..5b0fc156fc 100644
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: forward.cc,v 1.154 2007/04/06 04:50:06 rousskov Exp $
+ * $Id: forward.cc,v 1.155 2007/04/12 14:07:10 rousskov Exp $
  *
  * DEBUG: section 17    Request Forwarding
  * AUTHOR: Duane Wessels
@@ -99,10 +99,24 @@ FwdState::FwdState(int fd, StoreEntry * e, HttpRequest * r)
 
     ;
     EBIT_SET(e->flags, ENTRY_FWD_HDR_WAIT);
+}
+
+// Called once, right after object creation, when it is safe to set self
+void FwdState::start(Pointer aSelf) {
+    // Protect ourselves from being destroyed when the only Server pointing
+    // to us is gone (while we expect to talk to more Servers later).
+    // Once we set self, we are responsible for clearing it when we do not
+    // expect to talk to any servers.
+    self = aSelf; // refcounted
+
+    // We hope that either the store entry aborts or peer is selected.
+    // Otherwise we are going to leak our object.
 
-    self = this;	// refcounted
+    storeRegisterAbort(entry, FwdState::abort, this);
+    peerSelect(request, entry, fwdStartCompleteWrapper, this);
 
-    storeRegisterAbort(e, FwdState::abort, this);
+    // TODO: set self _after_ the peer is selected because we do not need 
+    // self until we start talking to some Server.
 }
 
 void
@@ -245,9 +259,8 @@ FwdState::fwdStart(int client_fd, StoreEntry *entry, HttpRequest *request)
         return;
 
     default:
-        FwdState *fwd = new FwdState(client_fd, entry, request);
-	fwd->flags.forward_completed = 0;
-        peerSelect(request, entry, fwdStartCompleteWrapper, fwd);
+        FwdState::Pointer fwd = new FwdState(client_fd, entry, request);
+        fwd->start(fwd);
         return;
     }
 
@@ -314,14 +327,6 @@ FwdState::complete()
 
         storeEntryReset(e);
 
-        /*
-         * Need to re-establish the self-reference here since we'll
-         * be trying to forward the request again.  Otherwise the
-         * ref count could go to zero before a connection is
-         * established.
-         */
-        self = this;	// refcounted
-
         startComplete(servers);
     } else {
         debug(17, 3) ("fwdComplete: not re-forwarding status %d\n",
@@ -330,6 +335,7 @@ FwdState::complete()
 	entry->complete();
 	if (server_fd < 0)
 		completed();
+        self = NULL; // refcounted
     }
 }
 
@@ -937,12 +943,6 @@ FwdState::dispatch()
             break;
         }
     }
-
-    /*
-     * remove our self-refcount now that we've handed off the request
-     * to a server-side module
-     */
-    self = NULL;
 }
 
 int
diff --git a/src/forward.h b/src/forward.h
index 16451ec041..19b7961480 100644
--- a/src/forward.h
+++ b/src/forward.h
@@ -22,7 +22,6 @@ class FwdState : public RefCountable
 
 public:
     typedef RefCount<FwdState> Pointer;
-    FwdState(int fd, StoreEntry *, HttpRequest *);
     ~FwdState();
     static void initModule();
     static void RegisterWithCacheManager(CacheManager & manager);
@@ -57,6 +56,10 @@ public:
     static void serversFree(FwdServer **);
 
 private:
+    // hidden for safer management of self; use static fwdStart
+    FwdState(int fd, StoreEntry *, HttpRequest *);
+    void start(Pointer aSelf);
+
     static void logReplyStatus(int tries, http_status status);
     void completed();