From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sat, 1 Jul 2017 09:59:16 +0000 (+1200)
Subject: Fix mgr query handoff from the original recipient to Coordinator.
X-Git-Tag: SQUID_3_5_28~23
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0910a2eb4763de33c899a87e96239c8784afcc08;p=thirdparty%2Fsquid.git

Fix mgr query handoff from the original recipient to Coordinator.

This bug has already been fixed once, in trunk r11164.1.61, but that fix
was accidentally undone shortly after, during significant cross-branch
merging activity combined with the Forwarder class split. The final
merge importing the associated code (trunk r11730) was buggy.

The bug (explained in r11164.1.61) leads to a race condition between

* Store notifying Server classes about the entry completion (which might
  trigger a bogus error message sent to the cache manager client while
  Coordinator sends its own valid response on the same connection!) and

* post-cleanup() connection closure handlers of Server classes silently
  closing everything (and leaving Coordinator the only responding
  process on that shared connection).

The bug probably was not noticed for so long because, evidently, the
latter actions tend to win in the current code.
---

diff --git a/src/ipc/Forwarder.h b/src/ipc/Forwarder.h
index 8cdd72ac44..20653cdf4f 100644
--- a/src/ipc/Forwarder.h
+++ b/src/ipc/Forwarder.h
@@ -47,12 +47,14 @@ protected:
     virtual void handleError();
     virtual void handleTimeout();
     virtual void handleException(const std::exception& e);
-    virtual void handleRemoteAck();
 
 private:
     static void RequestTimedOut(void* param);
     void requestTimedOut();
     void removeTimeoutEvent();
+
+    void handleRemoteAck();
+
     static AsyncCall::Pointer DequeueRequest(unsigned int requestId);
 
 protected:
diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc
index 2038b108d7..c890428e92 100644
--- a/src/mgr/Forwarder.cc
+++ b/src/mgr/Forwarder.cc
@@ -102,17 +102,6 @@ Mgr::Forwarder::noteCommClosed(const CommCloseCbParams& params)
     mustStop("commClosed");
 }
 
-/// called when Coordinator starts processing the request
-void
-Mgr::Forwarder::handleRemoteAck()
-{
-    Ipc::Forwarder::handleRemoteAck();
-
-    Must(entry != NULL);
-    EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
-    entry->complete();
-}
-
 /// send error page
 void
 Mgr::Forwarder::sendError(ErrorState *error)
diff --git a/src/mgr/Forwarder.h b/src/mgr/Forwarder.h
index 3224a999b1..a6b3c9dac4 100644
--- a/src/mgr/Forwarder.h
+++ b/src/mgr/Forwarder.h
@@ -40,7 +40,6 @@ protected:
     virtual void handleError();
     virtual void handleTimeout();
     virtual void handleException(const std::exception& e);
-    virtual void handleRemoteAck();
 
 private:
     void noteCommClosed(const CommCloseCbParams& params);
diff --git a/src/tests/stub_libmgr.cc b/src/tests/stub_libmgr.cc
index 97e1d2505e..32a21933b8 100644
--- a/src/tests/stub_libmgr.cc
+++ b/src/tests/stub_libmgr.cc
@@ -100,7 +100,6 @@ void Mgr::Forwarder::cleanup() STUB
 void Mgr::Forwarder::handleError() STUB
 void Mgr::Forwarder::handleTimeout() STUB
 void Mgr::Forwarder::handleException(const std::exception& e) STUB
-void Mgr::Forwarder::handleRemoteAck() STUB
 
 #include "mgr/FunAction.h"
 Mgr::Action::Pointer Mgr::FunAction::Create(const CommandPointer &cmd, OBJH *aHandler) STUB_RETVAL(dummyAction)