From: Eduard Bagdasaryan Date: Mon, 27 Nov 2023 03:38:17 +0000 (+0000) Subject: Deduplicate HelperServerClosed() (#1587) X-Git-Tag: SQUID_7_0_1~273 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0a958e4294dc3fa5526ce14346ace36f3eac28a5;p=thirdparty%2Fsquid.git 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 d7544f13af..e54da6f361 100644 --- 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(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(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 3df92e0a09..c74e36185b 100644 --- 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;