Converted helperOpenServers() to a virtual Helper::Client member (#1489)

addressing a TODO.

No Squid functionality changes are expected.

diff --git a/src/auth/basic/Config.cc b/src/auth/basic/Config.cc
index afbccfb237f7d60da797447fad970660ada6d58b..7154c5375e4f1037f4f0e27de11b5773a2218fea 100644 (file)
--- a/src/auth/basic/Config.cc
+++ b/src/auth/basic/Config.cc
@@ -312,7 +312,7 @@ Auth::Basic::Config::init(Auth::SchemeConfig *)
 
         basicauthenticators->ipc_type = IPC_STREAM;
 
-        helperOpenServers(basicauthenticators);
+        basicauthenticators->openSessions();
     }
 }
 

diff --git a/src/auth/digest/Config.cc b/src/auth/digest/Config.cc
index 46d028b30666818ee2859621830f0555f33fb672..4fc3666b830362a1628f621ef8aa4925c713abb0 100644 (file)
--- a/src/auth/digest/Config.cc
+++ b/src/auth/digest/Config.cc
@@ -533,7 +533,7 @@ Auth::Digest::Config::init(Auth::SchemeConfig *)
 
         digestauthenticators->ipc_type = IPC_STREAM;
 
-        helperOpenServers(digestauthenticators);
+        digestauthenticators->openSessions();
     }
 }
 

diff --git a/src/auth/negotiate/Config.cc b/src/auth/negotiate/Config.cc
index e1f5e2669bcdae336cf531b4c6322e2143cb732a..d44918557708ea18777c1a63bf9a30a148ef8060 100644 (file)
--- a/src/auth/negotiate/Config.cc
+++ b/src/auth/negotiate/Config.cc
@@ -102,7 +102,7 @@ Auth::Negotiate::Config::init(Auth::SchemeConfig *)
 
         negotiateauthenticators->ipc_type = IPC_STREAM;
 
-        helperStatefulOpenServers(negotiateauthenticators);
+        negotiateauthenticators->openSessions();
     }
 }
 

diff --git a/src/auth/ntlm/Config.cc b/src/auth/ntlm/Config.cc
index 0928a970efce4104e3a8dfda15b5b25771ea3754..58e226560a99e9eb4718f54582dd75f42ef1e677 100644 (file)
--- a/src/auth/ntlm/Config.cc
+++ b/src/auth/ntlm/Config.cc
@@ -101,7 +101,7 @@ Auth::Ntlm::Config::init(Auth::SchemeConfig *)
 
         ntlmauthenticators->ipc_type = IPC_STREAM;
 
-        helperStatefulOpenServers(ntlmauthenticators);
+        ntlmauthenticators->openSessions();
     }
 }
 

diff --git a/src/external_acl.cc b/src/external_acl.cc
index 04320ff4e8292e3000c625a5e739ef97fc2f3474..1895d7b22556a8c32e4092d0fd2c4cf4d0272566 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -1123,7 +1123,7 @@ externalAclInit(void)
 
         p->theHelper->addr = p->local_addr;
 
-        helperOpenServers(p->theHelper);
+        p->theHelper->openSessions();
     }
 
     externalAclRegisterWithCacheManager();

diff --git a/src/helper.cc b/src/helper.cc
index eed0324f1bdff8f6f6fb4ba2b638d8013be88c7a..1092a3731d089463cb26cc99559fb09250e1cbb7 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -193,7 +193,7 @@ helper_stateful_server::~helper_stateful_server()
 }
 
 void
-helperOpenServers(const Helper::Client::Pointer &hlp)
+Helper::Client::openSessions()
 {
     char *s;
     char *progname;
@@ -208,6 +208,8 @@ helperOpenServers(const Helper::Client::Pointer &hlp)
     int wfd;
     void * hIpc;
     wordlist *w;
+    // Helps reducing diff. TODO: remove
+    const auto hlp = this;
 
     if (hlp->cmdline == nullptr)
         return;
@@ -327,18 +329,15 @@ helperOpenServers(const Helper::Client::Pointer &hlp)
     helperKickQueue(hlp);
 }
 
-/**
- * DPW 2007-05-08
- *
- * helperStatefulOpenServers: create the stateful child helper processes
- */
 void
-helperStatefulOpenServers(const statefulhelper::Pointer &hlp)
+statefulhelper::openSessions()
 {
     char *shortname;
     const char *args[HELPER_MAX_ARGS+1]; // save space for a NULL terminator
     char fd_note_buf[FD_DESC_SZ];
     int nargs = 0;
+    // Helps reducing diff. TODO: remove
+    const auto hlp = this;
 
     if (hlp->cmdline == nullptr)
         return;
@@ -891,38 +890,30 @@ Helper::Client::handleFewerServers(const bool madeProgress)
 }
 
 void
-Helper::Session::HelperServerClosed(Session * const srv)
+Helper::Client::sessionClosed(SessionBase &session)
 {
-    const auto hlp = srv->parent;
-
     bool needsNewServers = false;
-    hlp->handleKilledServer(srv, needsNewServers);
+    handleKilledServer(&session, needsNewServers);
     if (needsNewServers) {
         debugs(80, DBG_IMPORTANT, "Starting new helpers");
-        helperOpenServers(hlp);
+        openSessions();
     }
+}
 
+void
+Helper::Session::HelperServerClosed(Session * const srv)
+{
+    srv->parent->sessionClosed(*srv);
     srv->dropQueued();
-
     delete srv;
 }
 
-// XXX: Almost duplicates Helper::Session::HelperServerClosed() because helperOpenServers() is not a virtual method of the `Helper::Client` class
-// TODO: Fix the `Helper::Client` class hierarchy to use virtual functions.
+// XXX: Essentially duplicates Helper::Session::HelperServerClosed() until we add SessionBase::helper().
 void
 helper_stateful_server::HelperServerClosed(helper_stateful_server *srv)
 {
-    const auto hlp = srv->parent;
-
-    bool needsNewServers = false;
-    hlp->handleKilledServer(srv, needsNewServers);
-    if (needsNewServers) {
-        debugs(80, DBG_IMPORTANT, "Starting new helpers");
-        helperStatefulOpenServers(hlp);
-    }
-
+    srv->parent->sessionClosed(*srv);
     srv->dropQueued();
-
     delete srv;
 }
 
@@ -1248,8 +1239,7 @@ Enqueue(Helper::Client * const hlp, Helper::Xaction * const r)
 
     /* do this first so idle=N has a chance to grow the child pool before it hits critical. */
     if (hlp->childs.needNew() > 0) {
-        debugs(84, DBG_CRITICAL, "Starting new " << hlp->id_name << " helpers...");
-        helperOpenServers(hlp);
+        hlp->openSessions();
         return;
     }
 
@@ -1277,8 +1267,7 @@ StatefulEnqueue(statefulhelper * hlp, Helper::Xaction * r)
 
     /* do this first so idle=N has a chance to grow the child pool before it hits critical. */
     if (hlp->childs.needNew() > 0) {
-        debugs(84, DBG_CRITICAL, "Starting new " << hlp->id_name << " helpers...");
-        helperStatefulOpenServers(hlp);
+        hlp->openSessions();
         return;
     }
 

diff --git a/src/helper.h b/src/helper.h
index a32eaf209cddda306f20de6f75978ddce26e967d..fbf2572e2fc892c360ec635b5687135759f0c57d 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -100,6 +100,13 @@ public:
     /// \param madeProgress whether the died helper(s) responded to any requests
     void handleFewerServers(bool madeProgress);
 
+    /// Starts required helper process(es).
+    /// 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;
@@ -156,6 +163,9 @@ public:
     /// undo reserveServer(), clear the reservation and kick the queue
     void cancelReservation(const Helper::ReservationId reservation);
 
+    /* Helper::Client API */
+    void openSessions() override;
+
 private:
     friend void helperStatefulSubmit(const statefulhelper::Pointer &, const char *buf, HLPCB *, void *cbData, const Helper::ReservationId &);
 
@@ -321,8 +331,6 @@ public:
     time_t reservationStart; ///< when the last `reservation` was made
 };
 
-void helperOpenServers(const Helper::Client::Pointer &);
-void helperStatefulOpenServers(const statefulhelper::Pointer &);
 void helperSubmit(const Helper::Client::Pointer &, const char *buf, HLPCB *, void *cbData);
 void helperStatefulSubmit(const statefulhelper::Pointer &, const char *buf, HLPCB *, void *cbData, uint64_t reservation);
 void helperShutdown(const Helper::Client::Pointer &);

diff --git a/src/redirect.cc b/src/redirect.cc
index aee5d79b7e91a9de22b694398819f7fd8205793f..ddf79018234437af2c218a871e61d49ff76c3b96 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -363,7 +363,7 @@ redirectInit(void)
         if (Config.onUrlRewriteTimeout.action == toutActUseConfiguredResponse)
             redirectors->onTimedOutResponse.assign(Config.onUrlRewriteTimeout.response);
 
-        helperOpenServers(redirectors);
+        redirectors->openSessions();
     }
 
     if (Config.Program.store_id) {
@@ -384,7 +384,7 @@ redirectInit(void)
 
         storeIds->retryBrokenHelper = true; // XXX: make this configurable ?
 
-        helperOpenServers(storeIds);
+        storeIds->openSessions();
     }
 
     if (Config.redirector_extras) {

diff --git a/src/ssl/helper.cc b/src/ssl/helper.cc
index d90f0c3183a83c65b583daf6379e494ff92e2855..1d37c5e776bb02bee7ae26b32b23858ad0476b4e 100644 (file)
--- a/src/ssl/helper.cc
+++ b/src/ssl/helper.cc
@@ -102,7 +102,7 @@ void Ssl::Helper::Init()
         }
         safe_free(tmp_begin);
     }
-    helperOpenServers(ssl_crtd);
+    ssl_crtd->openSessions();
 }
 
 void Ssl::Helper::Shutdown()
@@ -222,7 +222,7 @@ void Ssl::CertValidationHelper::Init()
         }
         xfree(tmp_begin);
     }
-    helperOpenServers(ssl_crt_validator);
+    ssl_crt_validator->openSessions();
 
     //WARNING: initializing static member in an object initialization method
     assert(HelperCache == nullptr);

diff --git a/src/tests/stub_helper.cc b/src/tests/stub_helper.cc
index 593ce43853e35506daa46fabda651ef44c2d0eb9..a0411d07116d9cbac6b080ed9171f2cc959b3b5f 100644 (file)
--- a/src/tests/stub_helper.cc
+++ b/src/tests/stub_helper.cc
@@ -16,9 +16,9 @@ void helperSubmit(const Helper::Client::Pointer &, const char *, HLPCB *, void *
 void helperStatefulSubmit(const statefulhelper::Pointer &, const char *, HLPCB *, void *, const Helper::ReservationId &) STUB
 Helper::Client::~Client() STUB
 void Helper::Client::packStatsInto(Packable *, const char *) const STUB
+void Helper::Client::openSessions() STUB
 
 void helperShutdown(const Helper::Client::Pointer &) STUB
 void helperStatefulShutdown(const statefulhelper::Pointer &) STUB
-void helperOpenServers(const Helper::Client::Pointer &) STUB
-void helperStatefulOpenServers(const statefulhelper::Pointer &) STUB
+void statefulhelper::openSessions() STUB