Bug #1981 fix: Tell FwdState that an unregistered and failed Server is

gone.

When a Server unregisters its fd (by calling FwdState::unregister) and
then fails, call FwdState::handleUnregisteredServerEnd to tell FwdState
that the server is done working, so that the transaction does not get
stuck waiting for the server to call FwdState::complete. The old code
would just set Server's FwdState pointer to NULL which was only enough
if FwdState::self was already NULL due to aborts.

This change fixed the bug in my limited ICAP tests.

The whole FwdState relationship with Servers needs a serious revision as
similar bugs probably do (or will) exist. We probably need to maintain a
state variable representing the relationship. The following Server
states are possible, at least: got valid FD and working, closed FD but
still working, stopped working (failure or success). FwdState needs to
be notified when we stopped working. Currently, when Server closes the
FD, deep down the Server stack, it may not be clear whether the Server
is still working or not.

Finally, it may be a good idea for FwdState to notify Server when
FwdState is aborted. Currently, the Server must check that the store
entry is still valid to notice the abort, and it sometimes forgets to do
that, causing store assertions.

diff --git a/src/forward.cc b/src/forward.cc
index 2ad70f34d1d0c8df08e25448ab7c05a5536ea01a..4a8f5de655d491c8fab7eba2088184cbee26eaba 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: forward.cc,v 1.165 2007/05/26 06:38:04 wessels Exp $
+ * $Id: forward.cc,v 1.166 2007/06/19 20:27:00 rousskov Exp $
  *
  * DEBUG: section 17    Request Forwarding
  * AUTHOR: Duane Wessels
@@ -497,6 +497,14 @@ FwdState::serverClosed(int fd)
     assert(server_fd == fd);
     server_fd = -1;
 
+    retryOrBail();
+}
+
+void
+FwdState::retryOrBail() {
+    if (!self) // we have aborted before the server called us back
+        return; // we are destroyed when the server clears its Pointer to us
+
     if (checkRetry()) {
         int originserver = (servers->_peer == NULL);
         debugs(17, 3, "fwdServerClosed: re-forwarding (" << n_tries << " tries, " << (squid_curtime - start_t) << " secs)");
@@ -535,6 +543,16 @@ FwdState::serverClosed(int fd)
     self = NULL;       // refcounted
 }
 
+// called by the server that failed after calling unregister()
+void
+FwdState::handleUnregisteredServerEnd()
+{
+    debugs(17, 2, "handleUnregisteredServerEnd: self=" << self <<
+        " err=" << err << ' ' << entry->url());
+    assert(server_fd < 0);
+    retryOrBail();
+}
+
 #if USE_SSL
 void
 FwdState::negotiateSSL(int fd)

diff --git a/src/forward.h b/src/forward.h
index 483af54ed8cdeb47b3893dd72b9388632025a67a..be43dfe6a11e4d3bf64b225f49f9202b57b729bf 100644 (file)
--- a/src/forward.h
+++ b/src/forward.h
@@ -32,6 +32,7 @@ public:
     void fail(ErrorState *err);
     void unregister(int fd);
     void complete();
+    void handleUnregisteredServerEnd();
     int reforward();
     bool reforwardableStatus(http_status s);
     void serverClosed(int fd);
@@ -62,6 +63,7 @@ private:
 
     static void logReplyStatus(int tries, http_status status);
     void completed();
+    void retryOrBail();
 
 #if WIP_FWD_LOG
 

diff --git a/src/ftp.cc b/src/ftp.cc
index 578d2d4d51f90d1e6df332ca68687a2b7fe3d06a..922a49d11630e93f3ca5f5706032d0edd07082dd 100644 (file)
--- a/src/ftp.cc
+++ b/src/ftp.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: ftp.cc,v 1.424 2007/05/29 13:31:39 amosjeffries Exp $
+ * $Id: ftp.cc,v 1.425 2007/06/19 20:27:00 rousskov Exp $
  *
  * DEBUG: section 9     File Transfer Protocol (FTP)
  * AUTHOR: Harvest Derived
@@ -3378,10 +3378,13 @@ FtpStateData::abortTransaction(const char *reason)
 {
     debugs(9,5,HERE << "aborting transaction for " << reason <<
         "; FD " << ctrl.fd << ", Data FD " << data.fd << ", this " << this);
-    if (ctrl.fd >= 0)
+    if (ctrl.fd >= 0) {
         comm_close(ctrl.fd);
-    else
-        delete this;
+        return;
+    }
+    
+    fwd->handleUnregisteredServerEnd();
+    delete this;
 }
 
 #if ICAP_CLIENT

diff --git a/src/http.cc b/src/http.cc
index a674aa1a4d5b9231a00ea40d400c5886e6430f6e..c5f50d6cda58a23b426d23b34ef67fdfd5edfb34 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: http.cc,v 1.524 2007/06/17 21:48:20 hno Exp $
+ * $Id: http.cc,v 1.525 2007/06/19 20:27:00 rousskov Exp $
  *
  * DEBUG: section 11    Hypertext Transfer Protocol (HTTP)
  * AUTHOR: Harvest Derived
@@ -1935,10 +1935,13 @@ HttpStateData::abortTransaction(const char *reason)
     debugs(11,5, HERE << "aborting transaction for " << reason <<
            "; FD " << fd << ", this " << this);
 
-    if (fd >= 0)
+    if (fd >= 0) {
         comm_close(fd);
-    else
-        delete this;
+        return;
+       }
+
+    fwd->handleUnregisteredServerEnd();
+    delete this;
 }
 
 void