Drop helpless helper requests (#1471)

When a helper has no server programs left (e.g., because all started
programs, if any, have quit and no new programs could be started), drop
all queued helper requests, so that their corresponding master
transactions do not get stuck.

Existing handleFewerServers() already handles this situation by killing
Squid, but we must drop queued requests in other places (that can be
found by locating code that adjusts childs.n_active) because:

* that existing Squid-killing code effectively excludes several use
  cases (e.g., it ignores common startup=0 helper configurations);

* handleFewerServers() may not be called in some relevant cases
  (e.g., when no new servers were needed after a reconfiguration);

* handleFewerServers() may be followed by a helperOpenServers() call
  (that may open new servers for handling queued requests); we do
  not want to kill queued requests in those cases.

Squid may recover and successfully start another helper later, but
transaction X progress must not rely on an unrelated transaction Y
actions: If transaction Y does not show up or does not result in a
successful helper program start, then transaction X will continue to be
queued at the helper level, possibly forever (even request timeouts are
handled by individual helper server objects that we lack here).

Also significantly simplified HelperServerClosed() implementations.

diff --git a/src/helper.cc b/src/helper.cc
index b2c09297da98ad4f87398d6e874f9c290d8f081a..d7544f13afc008e994f0bd25cb2821cf9ed9a3fb 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -801,6 +801,9 @@ helperShutdown(const Helper::Client::Pointer &hlp)
          */
         srv->closePipesSafely(hlp->id_name);
     }
+
+    Assure(!hlp->childs.n_active);
+    hlp->dropQueued();
 }
 
 void
@@ -854,15 +857,17 @@ Helper::Client::~Client()
 {
     /* note, don't free id_name, it probably points to static memory */
 
-    // TODO: if the queue is not empty it will leak Helper::Request's
-    if (!queue.empty())
-        debugs(84, DBG_CRITICAL, "WARNING: freeing " << id_name << " helper with " << stats.queue_size << " requests queued");
+    // A non-empty queue would leak Helper::Xaction objects, stalling any
+    // pending (and even future collapsed) transactions. To avoid stalling
+    // transactions, we must dropQueued(). We ought to do that when we
+    // discover that no progress is possible rather than here because
+    // reference counting may keep this object alive for a long time.
+    assert(queue.empty());
 }
 
 void
-Helper::Client::handleKilledServer(SessionBase * const srv, bool &needsNewServers)
+Helper::Client::handleKilledServer(SessionBase * const srv)
 {
-    needsNewServers = false;
     if (!srv->flags.shutdown) {
         assert(childs.n_active > 0);
         --childs.n_active;
@@ -872,9 +877,33 @@ Helper::Client::handleKilledServer(SessionBase * const srv, bool &needsNewServer
 
         if (childs.needNew() > 0) {
             srv->flags.shutdown = true;
-            needsNewServers = true;
+            openSessions();
         }
     }
+
+    if (!childs.n_active)
+        dropQueued();
+}
+
+void
+Helper::Client::dropQueued()
+{
+    if (queue.empty())
+        return;
+
+    Assure(!childs.n_active);
+    Assure(!GetFirstAvailable(this));
+
+    // no helper servers means nobody can advance our queued transactions
+
+    debugs(80, DBG_CRITICAL, "ERROR: Dropping " << queue.size() << ' ' <<
+           id_name << " helper requests due to lack of helper processes");
+    // similar to SessionBase::dropQueued()
+    while (const auto r = nextRequest()) {
+        r->reply.result = Helper::Unknown;
+        callBack(*r);
+        delete r;
+    }
 }
 
 void
@@ -897,21 +926,10 @@ Helper::Client::handleFewerServers(const bool madeProgress)
     }
 }
 
-void
-Helper::Client::sessionClosed(SessionBase &session)
-{
-    bool needsNewServers = false;
-    handleKilledServer(&session, needsNewServers);
-    if (needsNewServers) {
-        debugs(80, DBG_IMPORTANT, "Starting new helpers");
-        openSessions();
-    }
-}
-
 void
 Helper::Session::HelperServerClosed(Session * const srv)
 {
-    srv->parent->sessionClosed(*srv);
+    srv->parent->handleKilledServer(srv);
     srv->dropQueued(*srv->parent);
     delete srv;
 }
@@ -920,7 +938,7 @@ Helper::Session::HelperServerClosed(Session * const srv)
 void
 helper_stateful_server::HelperServerClosed(helper_stateful_server *srv)
 {
-    srv->parent->sessionClosed(*srv);
+    srv->parent->handleKilledServer(srv);
     srv->dropQueued(*srv->parent);
     delete srv;
 }
@@ -1521,6 +1539,9 @@ helperKickQueue(const Helper::Client::Pointer &hlp)
 
     while ((srv = GetFirstAvailable(hlp)) && (r = hlp->nextRequest()))
         helperDispatch(srv, r);
+
+    if (!hlp->childs.n_active)
+        hlp->dropQueued();
 }
 
 static void
@@ -1533,6 +1554,9 @@ helperStatefulKickQueue(const statefulhelper::Pointer &hlp)
         hlp->reserveServer(srv);
         helperStatefulDispatch(srv, r);
     }
+
+    if (!hlp->childs.n_active)
+        hlp->dropQueued();
 }
 
 static void

diff --git a/src/helper.h b/src/helper.h
index 8b6c2cd84093615923d0d6a00c26cdd43c0a9ff1..3df92e0a0906258329d7e951d6a85220f8352ddd 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -92,14 +92,18 @@ public:
 
     /// Updates internal statistics and starts new helper processes after
     /// an unexpected server exit
-    /// \param needsNewServers true if new helper(s) must be started, false otherwise
-    void handleKilledServer(SessionBase *, bool &needsNewServers);
+    void handleKilledServer(SessionBase *);
 
     /// Reacts to unexpected helper process death(s), including a failure to start helper(s)
     /// and an unexpected exit of a previously started helper. \sa handleKilledServer()
     /// \param madeProgress whether the died helper(s) responded to any requests
     void handleFewerServers(bool madeProgress);
 
+    /// satisfies all queued requests with a Helper::Unknown answer
+    /// \prec no existing servers will be able to process queued requests
+    /// \sa SessionBase::dropQueued()
+    void dropQueued();
+
     /// sends transaction response to the transaction initiator
     void callBack(Xaction &);
 
@@ -107,9 +111,6 @@ public:
     /// The caller is responsible for checking that new processes are needed.
     virtual void openSessions();
 
-    /// handles exited helper process
-    void sessionClosed(SessionBase &);
-
 public:
     wordlist *cmdline = nullptr;
     dlink_list servers;