Improve handling of client connections on shutdown

When Squid which are processing a lot of traffic, using persistent
client connections, or dealing with long duration requests are shutdown
they can exit with a lot of connections still open. The
shutdown_lifetime directive exists to allow time for existing
transactions to complete, but this is not always possible and has no
effect on idle connections.

The result is a large dump of aborted FD entries being logged as the TCP
sockets get abruptly reset. Potentially active transactions cache
objects being "corrupted" in the process.

Makes ConnStateData and its children implement Runner API callbacks
to receive signals about Squid shutdown. Which allows their close()
handlers to be run properly and make use of AsyncCalls API. Idle client
connections are closed immediately on the startShutdown() signal, so
their closure CPU cycles happens during the shutdown grace period.

An extra 0-delay event step is added to SignalEngine shutdown sequence
with a new Runner registry hook 'endingShutdown' is added to signal that
the shutdown_lifetime grace period is over for closure of active
transactions. All network FD sockets should be considered unusable for
read()/write() at that point since close handlers may have already been
scheduled by other Runners. AsyncCall's may still be scheduled to
release resources.

Also adds a DeregisterRunner() API action to remove Runners dynamically
from the registered set.

The Squid shutdown sequence is now:

* shutdown signal received:
 - listening sockets closed
 - idle client connections closed

* shutdown grace period ends:
 - remaining client connections closed

* shutdown finishes:
 - main signal and Async loop halted
 - all memory free'd

Server connections which are PINNED or in active use during the
endingShutdown execution will be closed cleanly as a side-effect of the
client closures. Otherwise there is no change (yet) to server connections
or other FD sockets behaviour on shutdown.

diff --git a/src/base/RunnersRegistry.cc b/src/base/RunnersRegistry.cc
index af7775e9e8a42d42cc9fe224589709398492afc1..fd7a01a15895739e491541e63789c0bc812b1cef 100644 (file)
--- a/src/base/RunnersRegistry.cc
+++ b/src/base/RunnersRegistry.cc
@@ -32,6 +32,14 @@ RegisterRunner(RegisteredRunner *rr)
     return runners.size();
 }
 
+int
+DeregisterRunner(RegisteredRunner *rr)
+{
+    Runners &runners = GetRunners();
+    runners.erase(rr);
+    return runners.size();
+}
+
 void
 RunRegistered(const RegisteredRunner::Method &m)
 {

diff --git a/src/base/RunnersRegistry.h b/src/base/RunnersRegistry.h
index b21c56f8f265e00c046bf439c26506a17c27fa5c..11312b1eb3d6c5a3892469c5d5ed0c7a13476417 100644 (file)
--- a/src/base/RunnersRegistry.h
+++ b/src/base/RunnersRegistry.h
@@ -68,6 +68,12 @@ public:
     /// Meant for cleanup and state saving that may require other modules.
     virtual void startShutdown() {}
 
+    /// Called after shutdown_lifetime grace period ends and before stopping
+    /// the main loop. At least one main loop iteration is guaranteed after
+    /// this call.
+    /// Meant for cleanup and state saving that may require other modules.
+    virtual void endingShutdown() {}
+
     /// Called after stopping the main loop and before releasing memory.
     /// Meant for quick/basic cleanup that does not require any other modules.
     virtual ~RegisteredRunner() {}
@@ -82,6 +88,9 @@ public:
 /// registers a given runner with the given registry and returns registry count
 int RegisterRunner(RegisteredRunner *rr);
 
+/// de-registers a given runner with the given registry and returns registry count
+int DeregisterRunner(RegisteredRunner *rr);
+
 /// Calls a given method of all runners.
 /// All runners are destroyed after the finishShutdown() call.
 void RunRegistered(const RegisteredRunner::Method &m);

diff --git a/src/client_side.cc b/src/client_side.cc
index 22abf07d55ffa5ecbefc688430f603d9997d743c..87c7ad31a14e73321f1c8f47c20ef32578c4c149 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -809,6 +809,7 @@ ConnStateData::swanSong()
 {
     debugs(33, 2, HERE << clientConnection);
     flags.readMore = false;
+    DeregisterRunner(this);
     clientdbEstablished(clientConnection->remote, -1);  /* decrement */
     assert(areAllContextsForThisConnection());
     freeAllContexts();
@@ -1879,6 +1880,32 @@ ConnStateData::abortRequestParsing(const char *const uri)
     return context;
 }
 
+void
+ConnStateData::startShutdown()
+{
+    // RegisteredRunner API callback - Squid has been shut down
+
+    // if connection is idle terminate it now,
+    // otherwise wait for grace period to end
+    if (getConcurrentRequestCount() == 0)
+        endingShutdown();
+}
+
+void
+ConnStateData::endingShutdown()
+{
+    // RegisteredRunner API callback - Squid shutdown grace period is over
+
+    // force the client connection to close immediately
+    // swanSong() in the close handler will cleanup.
+    if (Comm::IsConnOpen(clientConnection))
+        clientConnection->close();
+
+    // deregister now to ensure finalShutdown() does not kill us prematurely.
+    // fd_table purge will cleanup if close handler was not fast enough.
+    DeregisterRunner(this);
+}
+
 char *
 skipLeadingSpace(char *aString)
 {
@@ -3392,6 +3419,10 @@ ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) :
     transferProtocol = port->transport; // default to the *_port protocol= setting. may change later.
     log_addr = xact->tcpClient->remote;
     log_addr.applyMask(Config.Addrs.client_netmask);
+
+    // register to receive notice of Squid signal events
+    // which may affect long persisting client connections
+    RegisterRunner(this);
 }
 
 void

diff --git a/src/client_side.h b/src/client_side.h
index 5fedb7496318b2c77ee3f9da51d2517d2ac3e039..86573697430b7264cad121bf892e9290925dc03d 100644 (file)
--- a/src/client_side.h
+++ b/src/client_side.h
@@ -11,6 +11,7 @@
 #ifndef SQUID_CLIENTSIDE_H
 #define SQUID_CLIENTSIDE_H
 
+#include "base/RunnersRegistry.h"
 #include "clientStreamForward.h"
 #include "comm.h"
 #include "helper/forward.h"
@@ -167,7 +168,7 @@ class ServerBump;
  *
  * If the above can be confirmed accurate we can call this object PipelineManager or similar
  */
-class ConnStateData : public BodyProducer, public HttpControlMsgSink
+class ConnStateData : public BodyProducer, public HttpControlMsgSink, public RegisteredRunner
 {
 
 public:
@@ -418,6 +419,11 @@ public:
     /// client data which may need to forward as-is to server after an
     /// on_unsupported_protocol tunnel decision.
     SBuf preservedClientData;
+
+    /* Registered Runner API */
+    virtual void startShutdown();
+    virtual void endingShutdown();
+
 protected:
     void startDechunkingRequest();
     void finishDechunkingRequest(bool withSuccess);

diff --git a/src/main.cc b/src/main.cc
index 67459bb47ac7652f6f9d11ef0a879d703563bb93..ccf9dd4d82a3893a827830be937e19936f5073df 100644 (file)
--- a/src/main.cc
+++ b/src/main.cc
@@ -211,6 +211,18 @@ private:
             EventLoop::Running->stop();
     }
 
+    static void FinalShutdownRunners(void *) {
+        RunRegisteredHere(RegisteredRunner::endingShutdown);
+
+        // XXX: this should be a Runner.
+#if USE_AUTH
+        /* detach the auth components (only do this on full shutdown) */
+        Auth::Scheme::FreeAll();
+#endif
+
+        eventAdd("SquidTerminate", &StopEventLoop, NULL, 0, 1, false);
+    }
+
     void doShutdown(time_t wait);
     void handleStoppedChild();
 
@@ -269,10 +281,6 @@ SignalEngine::doShutdown(time_t wait)
 
         /* run the closure code which can be shared with reconfigure */
         serverConnectionsClose();
-#if USE_AUTH
-        /* detach the auth components (only do this on full shutdown) */
-        Auth::Scheme::FreeAll();
-#endif
 
         RunRegisteredHere(RegisteredRunner::startShutdown);
     }
@@ -281,7 +289,7 @@ SignalEngine::doShutdown(time_t wait)
     WIN32_svcstatusupdate(SERVICE_STOP_PENDING, (wait + 1) * 1000);
 #endif
 
-    eventAdd("SquidShutdown", &StopEventLoop, this, (double) (wait + 1), 1, false);
+    eventAdd("SquidShutdown", &FinalShutdownRunners, this, (double) (wait + 1), 1, false);
 }
 
 void