Fix a race condition where supervisord might ignore a signal. Closes #54.

CHANGES.txt

@@ -33,6 +33,10 @@ Next release
   send it to its whole process group instead to take care of possible
   children as well and not leave them behind.  Patch by Samuele Pedroni.
 
+- Fixed a race condition where supervisord might not act on a signal sent
+  to it.  Thanks to Adar Dembo for reporting the issue and supplying the
+  initial patch.
+
 3.0a10 (2011-03-30)
 -------------------
 

supervisor/options.py

@@ -357,7 +357,6 @@ class ServerOptions(Options):
     pidfile = None
     passwdfile = None
     nodaemon = None
-    signal = None
     environment = None
     httpservers = ()
     unlink_socketfiles = True
@@ -404,6 +403,7 @@ class ServerOptions(Options):
         self.pidhistory = {}
         self.process_group_configs = []
         self.parse_warnings = []
+        self.signal_receiver = SignalReceiver()
 
     def version(self, dummy):
         """Print version to stdout and exit(0).
@@ -1033,15 +1033,16 @@ class ServerOptions(Options):
         self.logger.close()
 
     def setsignals(self):
-        signal.signal(signal.SIGTERM, self.sigreceiver)
-        signal.signal(signal.SIGINT, self.sigreceiver)
-        signal.signal(signal.SIGQUIT, self.sigreceiver)
-        signal.signal(signal.SIGHUP, self.sigreceiver)
-        signal.signal(signal.SIGCHLD, self.sigreceiver)
-        signal.signal(signal.SIGUSR2, self.sigreceiver)
+        receive = self.signal_receiver.receive
+        signal.signal(signal.SIGTERM, receive)
+        signal.signal(signal.SIGINT, receive)
+        signal.signal(signal.SIGQUIT, receive)
+        signal.signal(signal.SIGHUP, receive)
+        signal.signal(signal.SIGCHLD, receive)
+        signal.signal(signal.SIGUSR2, receive)
 
-    def sigreceiver(self, sig, frame):
-        self.signal = sig
+    def get_signal(self):
+        return self.signal_receiver.get_signal()
 
     def openhttpservers(self, supervisord):
         try:
@@ -1850,6 +1851,21 @@ def _init_signames():
             d[v] = k
     _signames = d
 
+class SignalReceiver:
+    def __init__(self):
+        self._signals_recvd = []
+
+    def receive(self, sig, frame):
+        if sig not in self._signals_recvd:
+            self._signals_recvd.append(sig)
+
+    def get_signal(self):
+        if self._signals_recvd:
+            sig = self._signals_recvd.pop(0)
+        else:
+            sig = None
+        return sig
+
 # miscellaneous utility functions
 
 def expand(s, expansions, name):

supervisor/supervisord.py

@@ -291,8 +291,8 @@ class Supervisor:
                 self.reap() # keep reaping until no more kids to reap
 
     def handle_signal(self):
-        if self.options.signal:
-            sig, self.options.signal = self.options.signal, None
+        sig = self.options.get_signal()
+        if sig:
             if sig in (signal.SIGTERM, signal.SIGINT, signal.SIGQUIT):
                 self.options.logger.warn(
                     'received %s indicating exit request' % signame(sig))

supervisor/tests/base.py

@@ -39,7 +39,7 @@ class DummyOptions:
         self.directory = None
         self.waitpid_return = None, None
         self.kills = {}
-        self.signal = None
+        self._signal = None
         self.parent_pipes_closed = None
         self.child_pipes_closed = None
         self.forkpid = 0
@@ -102,6 +102,9 @@ class DummyOptions:
     def setsignals(self):
         self.signals_set = True
 
+    def get_signal(self):
+        return self._signal
+
     def get_socket_map(self):
         return self.socket_map
 

supervisor/tests/test_options.py

@@ -470,6 +470,14 @@ class ServerOptionsTests(unittest.TestCase):
         instance = self._makeOne()
         self.assertEqual(os.getpid(), instance.get_pid())
 
+    def test_get_signal_delegates_to_signal_receiver(self):
+        instance = self._makeOne()
+        instance.signal_receiver.receive(signal.SIGTERM, None)
+        instance.signal_receiver.receive(signal.SIGCHLD, None)
+        self.assertEqual(instance.get_signal(), signal.SIGTERM)
+        self.assertEqual(instance.get_signal(), signal.SIGCHLD)
+        self.assertEqual(instance.get_signal(), None)
+
     def test_check_execv_args_cant_find_command(self):
         instance = self._makeOne()
         from supervisor.options import NotFound
@@ -1499,6 +1507,39 @@ class FastCGIGroupConfigTests(unittest.TestCase):
         self.assertTrue(instance1 != instance2)
         self.assertFalse(instance1 == instance2)
 
+class SignalReceiverTests(unittest.TestCase):
+    def test_returns_None_initially(self):
+        from supervisor.options import SignalReceiver
+        sr = SignalReceiver()
+        self.assertEquals(sr.get_signal(), None)
+
+    def test_returns_signals_in_order_received(self):
+        from supervisor.options import SignalReceiver
+        sr = SignalReceiver()
+        sr.receive(signal.SIGTERM, 'frame')
+        sr.receive(signal.SIGCHLD, 'frame')
+        self.assertEquals(sr.get_signal(), signal.SIGTERM)
+        self.assertEquals(sr.get_signal(), signal.SIGCHLD)
+        self.assertEquals(sr.get_signal(), None)
+
+    def test_does_not_queue_duplicate_signals(self):
+        from supervisor.options import SignalReceiver
+        sr = SignalReceiver()
+        sr.receive(signal.SIGTERM, 'frame')
+        sr.receive(signal.SIGTERM, 'frame')
+        self.assertEquals(sr.get_signal(), signal.SIGTERM)
+        self.assertEquals(sr.get_signal(), None)
+
+    def test_queues_again_after_being_emptied(self):
+        from supervisor.options import SignalReceiver
+        sr = SignalReceiver()
+        sr.receive(signal.SIGTERM, 'frame')
+        self.assertEquals(sr.get_signal(), signal.SIGTERM)
+        self.assertEquals(sr.get_signal(), None)
+        sr.receive(signal.SIGCHLD, 'frame')
+        self.assertEquals(sr.get_signal(), signal.SIGCHLD)
+        self.assertEquals(sr.get_signal(), None)
+
 class UtilFunctionsTests(unittest.TestCase):
     def test_make_namespec(self):
         from supervisor.options import make_namespec

supervisor/tests/test_supervisord.py

@@ -137,7 +137,7 @@ class SupervisordTests(unittest.TestCase):
 
     def test_handle_sigterm(self):
         options = DummyOptions()
-        options.signal = signal.SIGTERM
+        options._signal = signal.SIGTERM
         supervisord = self._makeOne(options)
         supervisord.handle_signal()
         self.assertEqual(supervisord.options.mood, -1)
@@ -146,7 +146,7 @@ class SupervisordTests(unittest.TestCase):
 
     def test_handle_sigint(self):
         options = DummyOptions()
-        options.signal = signal.SIGINT
+        options._signal = signal.SIGINT
         supervisord = self._makeOne(options)
         supervisord.handle_signal()
         self.assertEqual(supervisord.options.mood, -1)
@@ -155,7 +155,7 @@ class SupervisordTests(unittest.TestCase):
 
     def test_handle_sigquit(self):
         options = DummyOptions()
-        options.signal = signal.SIGQUIT
+        options._signal = signal.SIGQUIT
         supervisord = self._makeOne(options)
         supervisord.handle_signal()
         self.assertEqual(supervisord.options.mood, -1)
@@ -164,7 +164,7 @@ class SupervisordTests(unittest.TestCase):
 
     def test_handle_sighup(self):
         options = DummyOptions()
-        options.signal = signal.SIGHUP
+        options._signal = signal.SIGHUP
         supervisord = self._makeOne(options)
         supervisord.handle_signal()
         self.assertEqual(supervisord.options.mood, 0)
@@ -173,7 +173,7 @@ class SupervisordTests(unittest.TestCase):
 
     def test_handle_sigusr2(self):
         options = DummyOptions()
-        options.signal = signal.SIGUSR2
+        options._signal = signal.SIGUSR2
         pconfig1 = DummyPConfig(options, 'process1', 'process1','/bin/process1')
         from supervisor.process import ProcessStates
         process1 = DummyProcess(pconfig1, state=ProcessStates.STOPPING)
@@ -191,7 +191,7 @@ class SupervisordTests(unittest.TestCase):
 
     def test_handle_unknown_signal(self):
         options = DummyOptions()
-        options.signal = signal.SIGUSR1
+        options._signal = signal.SIGUSR1
         supervisord = self._makeOne(options)
         supervisord.handle_signal()
         self.assertEqual(supervisord.options.mood, 1)