dnsdist: Fix a hang when removing a server with more than one socket

There was a lock starvation issue when removing a server with more
than one socket in use (`sockets` greater than 1 on the corresponding
`newServer` directive), because the mutex protecting the sockets array
would never be released long enough by the responder thread to allow
the thread stopping the server to acquire it.
This commit fixes that by marking the server as stopped right away,
before acquiring the lock, and also making sure that the responder
thread is woken up regularly (every second, even without any query
to process) and that it checks whether the server has been stopped
just after that.

The issue was introduced in be55a20ce9bb7140071279d70bcb460f1f2b7b7d,
and backported to 1.5.1 in f0d48318cce0dd80ae73c529362bdb2921d8c5c9.

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 39cb2e197e4170eb3b6a8396211933b5759c320d..c6f01ee20219ea006203769090bf6fbf03900c1f 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -535,7 +535,7 @@ static void pickBackendSocketsReadyForReceiving(const std::shared_ptr<Downstream
 
   {
     std::lock_guard<std::mutex> lock(state->socketsLock);
-    state->mplexer->getAvailableFDs(ready, -1);
+    state->mplexer->getAvailableFDs(ready, 1000);
   }
 }
 
@@ -555,10 +555,14 @@ try {
   std::vector<int> sockets;
   sockets.reserve(dss->sockets.size());
 
-  for(; !dss->isStopped(); ) {
+  for(;;) {
     dnsheader* dh = reinterpret_cast<struct dnsheader*>(packet);
     try {
       pickBackendSocketsReadyForReceiving(dss, sockets);
+      if (dss->isStopped()) {
+        break;
+      }
+
       for (const auto& fd : sockets) {
         ssize_t got = recv(fd, packet, sizeof(packet), 0);
         char * response = packet;

diff --git a/pdns/dnsdistdist/dnsdist-backend.cc b/pdns/dnsdistdist/dnsdist-backend.cc
index 2eb19a8c59d343105f130dfc64443803e0f47454..091839b414ce146cf499164c104f7484259b9679 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-backend.cc
+++ b/pdns/dnsdistdist/dnsdist-backend.cc
@@ -26,8 +26,8 @@
 bool DownstreamState::reconnect()
 {
   std::unique_lock<std::mutex> tl(connectLock, std::try_to_lock);
-  if (!tl.owns_lock()) {
-    /* we are already reconnecting */
+  if (!tl.owns_lock() || isStopped()) {
+    /* we are already reconnecting or stopped anyway */
     return false;
   }
 
@@ -101,14 +101,17 @@ bool DownstreamState::reconnect()
 
 void DownstreamState::stop()
 {
-  std::unique_lock<std::mutex> tl(connectLock);
-  std::lock_guard<std::mutex> slock(socketsLock);
   d_stopped = true;
 
-  for (auto& fd : sockets) {
-    if (fd != -1) {
-      /* shutdown() is needed to wake up recv() in the responderThread */
-      shutdown(fd, SHUT_RDWR);
+  {
+    std::lock_guard<std::mutex> tl(connectLock);
+    std::lock_guard<std::mutex> slock(socketsLock);
+
+    for (auto& fd : sockets) {
+      if (fd != -1) {
+        /* shutdown() is needed to wake up recv() in the responderThread */
+        shutdown(fd, SHUT_RDWR);
+      }
     }
   }
 }