From 569605b9dce14d2543722dfad145fa40248b268c Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Sat, 18 Nov 2023 19:06:24 +0000 Subject: [PATCH] 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. --- src/helper.cc | 62 +++++++++++++++++++++++++++++++++++---------------- src/helper.h | 11 ++++----- 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/src/helper.cc b/src/helper.cc index b2c09297da..d7544f13af 100644 --- 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 8b6c2cd840..3df92e0a09 100644 --- 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; -- 2.47.2