From: Amos Jeffries <squid3@treenet.co.nz>
Date: Mon, 3 Oct 2016 04:33:08 +0000 (+1300)
Subject: Convert DNS shutdown to use a registered runner
X-Git-Tag: SQUID_4_0_15~18
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ee58de203e0ea3221933a35e39d7b31e9e442179;p=thirdparty%2Fsquid.git

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 df27aad893..e410051d42 100644
--- 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 6aae2aee82..c49906c384 100644
--- 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 2d39589274..a89b024ae0 100644
--- 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 da80de42ec..588aed0b44 100644
--- 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