Deduplicate HelperServerClosed() (#1587)

Also removed helper from Helper::SessionBase method parameters since
that base class now has access to the helper object.

diff --git a/src/helper.cc b/src/helper.cc
index d7544f13afc008e994f0bd25cb2821cf9ed9a3fb..e54da6f361f173de418af82e28ff5358c7eb1d48 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -69,7 +69,7 @@ Helper::SessionBase::initStats()
 }
 
 void
-Helper::SessionBase::closePipesSafely(const char * const id_name)
+Helper::SessionBase::closePipesSafely()
 {
 #if _SQUID_WINDOWS_
     shutdown(writePipe->fd, SD_BOTH);
@@ -86,18 +86,16 @@ Helper::SessionBase::closePipesSafely(const char * const id_name)
     if (hIpc) {
         if (WaitForSingleObject(hIpc, 5000) != WAIT_OBJECT_0) {
             getCurrentTime();
-            debugs(84, DBG_IMPORTANT, "WARNING: " << id_name <<
+            debugs(84, DBG_IMPORTANT, "WARNING: " << helper().id_name <<
                    " #" << index << " (PID " << (long int)pid << ") didn't exit in 5 seconds");
         }
         CloseHandle(hIpc);
     }
-#else
-    (void)id_name;
 #endif
 }
 
 void
-Helper::SessionBase::closeWritePipeSafely(const char * const id_name)
+Helper::SessionBase::closeWritePipeSafely()
 {
 #if _SQUID_WINDOWS_
     shutdown(writePipe->fd, (readPipe->fd == writePipe->fd ? SD_BOTH : SD_SEND));
@@ -112,25 +110,23 @@ Helper::SessionBase::closeWritePipeSafely(const char * const id_name)
     if (hIpc) {
         if (WaitForSingleObject(hIpc, 5000) != WAIT_OBJECT_0) {
             getCurrentTime();
-            debugs(84, DBG_IMPORTANT, "WARNING: " << id_name <<
+            debugs(84, DBG_IMPORTANT, "WARNING: " << helper().id_name <<
                    " #" << index << " (PID " << (long int)pid << ") didn't exit in 5 seconds");
         }
         CloseHandle(hIpc);
     }
-#else
-    (void)id_name;
 #endif
 }
 
 void
-Helper::SessionBase::dropQueued(Client &client)
+Helper::SessionBase::dropQueued()
 {
     while (!requests.empty()) {
         // XXX: re-schedule these on another helper?
         const auto r = requests.front();
         requests.pop_front();
         r->reply.result = Helper::Unknown;
-        client.callBack(*r);
+        helper().callBack(*r);
         delete r;
     }
 }
@@ -155,7 +151,7 @@ Helper::Session::~Session()
     }
 
     if (Comm::IsConnOpen(writePipe))
-        closeWritePipeSafely(parent->id_name);
+        closeWritePipeSafely();
 
     dlinkDelete(&link, &parent->servers);
 
@@ -166,9 +162,9 @@ Helper::Session::~Session()
 }
 
 void
-Helper::Session::dropQueued(Client &client)
+Helper::Session::dropQueued()
 {
-    SessionBase::dropQueued(client);
+    SessionBase::dropQueued();
     requestsIndex.clear();
 }
 
@@ -176,7 +172,7 @@ helper_stateful_server::~helper_stateful_server()
 {
     /* TODO: walk the local queue of requests and carry them all out */
     if (Comm::IsConnOpen(writePipe))
-        closeWritePipeSafely(parent->id_name);
+        closeWritePipeSafely();
 
     parent->cancelReservation(reservationId);
 
@@ -298,7 +294,9 @@ Helper::Client::openSessions()
         if (wfd != rfd)
             commSetNonBlocking(wfd);
 
-        AsyncCall::Pointer closeCall = asyncCall(5,4, "Helper::Session::HelperServerClosed", cbdataDialer(Helper::Session::HelperServerClosed, srv));
+        AsyncCall::Pointer closeCall = asyncCall(5, 4, "Helper::Session::HelperServerClosed", cbdataDialer(SessionBase::HelperServerClosed,
+                    static_cast<Helper::SessionBase *>(srv)));
+
         comm_add_close_handler(rfd, closeCall);
 
         if (hlp->timeout && hlp->childs.concurrency) {
@@ -430,7 +428,9 @@ statefulhelper::openSessions()
         if (wfd != rfd)
             commSetNonBlocking(wfd);
 
-        AsyncCall::Pointer closeCall = asyncCall(5,4, "helper_stateful_server::HelperServerClosed", cbdataDialer(helper_stateful_server::HelperServerClosed, srv));
+        AsyncCall::Pointer closeCall = asyncCall(5, 4, "helper_stateful_server::HelperServerClosed", cbdataDialer(Helper::SessionBase::HelperServerClosed,
+                    static_cast<Helper::SessionBase *>(srv)));
+
         comm_add_close_handler(rfd, closeCall);
 
         AsyncCall::Pointer call = commCbCall(5,4, "helperStatefulHandleRead",
@@ -799,7 +799,7 @@ helperShutdown(const Helper::Client::Pointer &hlp)
         /* the rest of the details is dealt with in the helperServerFree
          * close handler
          */
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
     }
 
     Assure(!hlp->childs.n_active);
@@ -849,7 +849,7 @@ helperStatefulShutdown(const statefulhelper::Pointer &hlp)
         /* the rest of the details is dealt with in the helperStatefulServerFree
          * close handler
          */
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
     }
 }
 
@@ -927,19 +927,10 @@ Helper::Client::handleFewerServers(const bool madeProgress)
 }
 
 void
-Helper::Session::HelperServerClosed(Session * const srv)
-{
-    srv->parent->handleKilledServer(srv);
-    srv->dropQueued(*srv->parent);
-    delete srv;
-}
-
-// XXX: Essentially duplicates Helper::Session::HelperServerClosed() until we add SessionBase::helper().
-void
-helper_stateful_server::HelperServerClosed(helper_stateful_server *srv)
+Helper::SessionBase::HelperServerClosed(SessionBase * const srv)
 {
-    srv->parent->handleKilledServer(srv);
-    srv->dropQueued(*srv->parent);
+    srv->helper().handleKilledServer(srv);
+    srv->dropQueued();
     delete srv;
 }
 
@@ -974,7 +965,7 @@ helperReturnBuffer(Helper::Session * srv, const Helper::Client::Pointer &hlp, ch
             debugs(84, DBG_IMPORTANT, "ERROR: Disconnecting from a " <<
                    "helper that overflowed " << srv->rbuf_sz << "-byte " <<
                    "Squid input buffer: " << hlp->id_name << " #" << srv->index);
-            srv->closePipesSafely(hlp->id_name);
+            srv->closePipesSafely();
             return;
         }
 
@@ -1021,7 +1012,7 @@ helperReturnBuffer(Helper::Session * srv, const Helper::Client::Pointer &hlp, ch
     if (!srv->flags.shutdown) {
         helperKickQueue(hlp);
     } else if (!srv->flags.closing && !srv->stats.pending) {
-        srv->closeWritePipeSafely(srv->parent->id_name);
+        srv->closeWritePipeSafely();
     }
 }
 
@@ -1043,7 +1034,7 @@ helperHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len, Comm::
     debugs(84, 5, "helperHandleRead: " << len << " bytes from " << hlp->id_name << " #" << srv->index);
 
     if (flag != Comm::OK || len == 0) {
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
         return;
     }
 
@@ -1059,7 +1050,7 @@ helperHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len, Comm::
 
         srv->roffset = 0;
         srv->rbuf[0] = '\0';
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
         return;
     }
 
@@ -1163,7 +1154,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
            hlp->id_name << " #" << srv->index);
 
     if (flag != Comm::OK || len == 0) {
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
         return;
     }
 
@@ -1178,7 +1169,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
                " bytes '" << srv->rbuf << "'");
 
         srv->roffset = 0;
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
         return;
     }
 
@@ -1200,7 +1191,7 @@ helperStatefulHandleRead(const Comm::ConnectionPointer &conn, char *, size_t len
         debugs(84, DBG_IMPORTANT, "ERROR: Disconnecting from a " <<
                "helper that overflowed " << srv->rbuf_sz << "-byte " <<
                "Squid input buffer: " << hlp->id_name << " #" << srv->index);
-        srv->closePipesSafely(hlp->id_name);
+        srv->closePipesSafely();
         return;
     }
     /**
@@ -1565,7 +1556,7 @@ helperStatefulServerDone(helper_stateful_server * srv)
     if (!srv->flags.shutdown) {
         helperStatefulKickQueue(srv->parent);
     } else if (!srv->flags.closing && !srv->reserved() && !srv->stats.pending) {
-        srv->closeWritePipeSafely(srv->parent->id_name);
+        srv->closeWritePipeSafely();
         return;
     }
 }

diff --git a/src/helper.h b/src/helper.h
index 3df92e0a0906258329d7e951d6a85220f8352ddd..c74e36185b83c52a0308b4c147f5eae7a3762e3b 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -194,27 +194,29 @@ class SessionBase: public CbdataParent
 public:
     ~SessionBase() override;
 
+    /// close handler to handle exited server processes
+    static void HelperServerClosed(SessionBase *);
+
     /** Closes pipes to the helper safely.
      * Handles the case where the read and write pipes are the same FD.
-     *
-     * \param name displayed for the helper being shutdown if logging an error
      */
-    void closePipesSafely(const char *name);
+    void closePipesSafely();
 
     /** Closes the reading pipe.
      * If the read and write sockets are the same the write pipe will
      * also be closed. Otherwise its left open for later handling.
-     *
-     * \param name displayed for the helper being shutdown if logging an error
      */
-    void closeWritePipeSafely(const char *name);
+    void closeWritePipeSafely();
 
     // TODO: Teach each child to report its child-specific state instead.
     /// whether the server is locked for exclusive use by a client
     virtual bool reserved() = 0;
 
+    /// our creator (parent) object
+    virtual Client &helper() const = 0;
+
     /// dequeues and sends an Unknown answer to all queued requests
-    virtual void dropQueued(Client &);
+    virtual void dropQueued();
 
 public:
     /// Helper program identifier; does not change when contents do,
@@ -296,13 +298,11 @@ public:
 
     /* SessionBase API */
     bool reserved() override {return false;}
-    void dropQueued(Client &) override;
+    void dropQueued() override;
+    Client &helper() const override { return *parent; }
 
     /// Read timeout handler
     static void requestTimeout(const CommTimeoutCbParams &io);
-
-    /// close handler to handle exited server processes
-    static void HelperServerClosed(Session *);
 };
 
 } // namespace Helper
@@ -319,11 +319,9 @@ public:
     void reserve();
     void clearReservation();
 
-    /* HelperServerBase API */
+    /* Helper::SessionBase API */
     bool reserved() override {return reservationId.reserved();}
-
-    /// close handler to handle exited server processes
-    static void HelperServerClosed(helper_stateful_server *srv);
+    Helper::Client &helper() const override { return *parent; }
 
     statefulhelper::Pointer parent;