Convert DNS shutdown to use a registered runner

This patch adds a Runner to manage the DNS component state on shutdown
(and the matching reconfigure port closures). The actions taken on
reconfigure and shutdown are not changed, just their timing.

Visible differences are now that Squid logs when DNS ports are being
closed (and the reason), also that the still-open FD table report no
longer lists the DNS listening FD as being open on shutdown when
comm_exit() is called and forces all fd_table entries to close.

NP: I have not moved the Dns::Init() operations to the runner at this
stage because it needs some deeper analysis and redesign as to which
Squid processes DNS is enabled/initialized. Currently everything from
coordinator to Diskers enable the internal-DNS state.

One BUG found but not fixed:
 DNS sockets being closed during reconfigure produces a race between
 any already active connections (or ones received between closing DNS
 sockets and server listening sockets) and the reconfigure completing
 (Runner syncConfig() being run). Transactions which loose this race
 will produce DNS timeouts (or whatever the caller set) as the queries
 never get queued to be re-tried after the DNS sockets are re-opened.

diff --git a/src/base/RunnersRegistry.h b/src/base/RunnersRegistry.h
index df27aad893ba84cd155e60b2ce0565ba2a81216b..e410051d42747c9392ae7715e47ac4df6bc31ab8 100644 (file)
--- a/src/base/RunnersRegistry.h
+++ b/src/base/RunnersRegistry.h
@@ -57,6 +57,11 @@ public:
 
     /* Reconfiguration events */
 
+    /// Called after receiving a reconfigure request and before parsing squid.conf.
+    /// Meant for modules that need to prepare for their configuration being changed
+    /// [outside their control]. The changes end with the syncConfig() event.
+    virtual void startReconfigure() {}
+
     /// Called after parsing squid.conf during reconfiguration.
     /// Meant for adjusting the module state based on configuration changes.
     virtual void syncConfig() {}

diff --git a/src/dns/forward.h b/src/dns/forward.h
index 6aae2aee8233e47f7c2c863f3046f017706ef150..c49906c3841c3a68c2b1a145ca7359b7f7ec96f5 100644 (file)
--- a/src/dns/forward.h
+++ b/src/dns/forward.h
@@ -22,7 +22,6 @@ namespace Dns
 class LookupDetails;
 
 void Init(void);
-void Shutdown(void);
 
 } // namespace Dns
 

diff --git a/src/dns_internal.cc b/src/dns_internal.cc
index 2d395892740353e1d4fd238de03c8207b2046cfe..a89b024ae0add6a3c57649a0c5a757e729709d81 100644 (file)
--- a/src/dns_internal.cc
+++ b/src/dns_internal.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "base/InstanceId.h"
+#include "base/RunnersRegistry.h"
 #include "comm.h"
 #include "comm/Connection.h"
 #include "comm/ConnOpener.h"
@@ -209,6 +210,22 @@ struct _ns {
     nsvc *vc;
 };
 
+namespace Dns
+{
+
+/// manage DNS internal component
+class ConfigRr : public RegisteredRunner
+{
+public:
+    /* RegisteredRunner API */
+    virtual void startReconfigure() override;
+    virtual void endingShutdown() override;
+};
+
+RunnerRegistrationEntry(ConfigRr);
+
+} // namespace Dns
+
 struct _sp {
     char domain[NS_MAXDNAME];
     int queries;
@@ -903,7 +920,7 @@ nsvc::~nsvc()
 {
     delete queue;
     delete msg;
-    if (ns < nns) // XXX: Dns::Shutdown may have freed nameservers[]
+    if (ns < nns) // XXX: idnsShutdownAndFreeState may have freed nameservers[]
         nameservers[ns].vc = NULL;
 }
 
@@ -962,6 +979,13 @@ idnsSendQueryVC(idns_query * q, int nsn)
 static void
 idnsSendQuery(idns_query * q)
 {
+    // XXX: DNS sockets get closed during reconfigure produces a race between
+    // any already active connections (or ones received between closing DNS
+    // sockets and server listening sockets) and the reconfigure completing
+    // (Runner syncConfig() being run). Transactions which loose this race will
+    // produce DNS timeouts (or whatever the caller set) as their queries never
+    // get queued to be re-tried after the DNS socekts are re-opened.
+
     if (DnsSocketA < 0 && DnsSocketB < 0) {
         debugs(78, DBG_IMPORTANT, "WARNING: idnsSendQuery: Can't send query, no DNS socket!");
         return;
@@ -1641,12 +1665,14 @@ Dns::Init(void)
     Mgr::RegisterAction("idns", "Internal DNS Statistics", idnsStats, 0, 1);
 }
 
-void
-Dns::Shutdown(void)
+static void
+idnsShutdownAndFreeState(const char *reason)
 {
     if (DnsSocketA < 0 && DnsSocketB < 0)
         return;
 
+    debugs(78, 2, reason << ": Closing DNS sockets");
+
     if (DnsSocketA >= 0 ) {
         comm_close(DnsSocketA);
         DnsSocketA = -1;
@@ -1669,6 +1695,18 @@ Dns::Shutdown(void)
     idnsFreeSearchpath();
 }
 
+void
+Dns::ConfigRr::endingShutdown()
+{
+    idnsShutdownAndFreeState("Shutdown");
+}
+
+void
+Dns::ConfigRr::startReconfigure()
+{
+    idnsShutdownAndFreeState("Reconfigure");
+}
+
 static int
 idnsCachedLookup(const char *key, IDNSCB * callback, void *data)
 {

diff --git a/src/main.cc b/src/main.cc
index da80de42ec23cee0444e17dd74bc02db17e19557..588aed0b445faf59121fb6c9797963a94d006007 100644 (file)
--- a/src/main.cc
+++ b/src/main.cc
@@ -851,13 +851,14 @@ mainReconfigureStart(void)
     debugs(1, DBG_IMPORTANT, "Reconfiguring Squid Cache (version " << version_string << ")...");
     reconfiguring = 1;
 
+    RunRegisteredHere(RegisteredRunner::startReconfigure);
+
     // Initiate asynchronous closing sequence
     serverConnectionsClose();
     icpClosePorts();
 #if USE_HTCP
     htcpClosePorts();
 #endif
-    Dns::Shutdown();
 #if USE_SSL_CRTD
     Ssl::Helper::GetInstance()->Shutdown();
 #endif
@@ -2006,7 +2007,6 @@ SquidShutdown()
 #endif
 
     debugs(1, DBG_IMPORTANT, "Shutting down...");
-    Dns::Shutdown();
 #if USE_SSL_CRTD
     Ssl::Helper::GetInstance()->Shutdown();
 #endif