Rev 6193: serve_bzr wasn't properly cleaning up the new _on_sighup dict, etc. in http://bazaar.launchpad.net/~jameinel/bzr/2.5-soft-hangup-795025

[John Arbash Meinel]

At http://bazaar.launchpad.net/~jameinel/bzr/2.5-soft-hangup-795025

------------------------------------------------------------
revno: 6193
revision-id: john at arbash-meinel.com-20110923181128-d0qrj4u1celg9gkn
parent: john at arbash-meinel.com-20110923174515-8ygrre1lrefev9kr
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: 2.5-soft-hangup-795025
timestamp: Fri 2011-09-23 20:11:28 +0200
message:
  serve_bzr wasn't properly cleaning up the new _on_sighup dict, etc.
  So add a bzrlib.smart.signals.restore_sighup_handler() and add it as a
  cleanup that needs to be run in BzrServerFactory.tear_down().
  
  Change install_sighup_handler so that it still does something on Windows, since
  we can use it to test that BzrServerFactory interacts with it appropriately.
  Next we need to test that 'bzr serve --inet' will respond to SIGHUP correctly.
-------------- next part --------------
=== modified file 'bzrlib/smart/server.py'
--- a/bzrlib/smart/server.py	2011-09-23 17:08:11 +0000
+++ b/bzrlib/smart/server.py	2011-09-23 18:11:28 +0000
@@ -463,7 +463,10 @@
         self.cleanups.append(restore_default_ui_factory_and_lockdir_timeout)
         ui.ui_factory = ui.SilentUIFactory()
         lockdir._DEFAULT_TIMEOUT_SECONDS = 0
-        signals.install_sighup_handler()
+        orig = signals.install_sighup_handler()
+        def restore_signals():
+            signals.restore_sighup_handler(orig)
+        self.cleanups.append(restore_signals)
 
     def set_up(self, transport, host, port, inet, timeout):
         self._make_backing_transport(transport)
@@ -474,6 +477,7 @@
         for cleanup in reversed(self.cleanups):
             cleanup()
 
+
 def serve_bzr(transport, host=None, port=None, inet=False, timeout=None):
     """This is the default implementation of 'bzr serve'.
 

=== modified file 'bzrlib/smart/signals.py'
--- a/bzrlib/smart/signals.py	2011-09-23 11:43:51 +0000
+++ b/bzrlib/smart/signals.py	2011-09-23 18:11:28 +0000
@@ -56,10 +56,11 @@
     if getattr(signal, "SIGHUP", None) is None:
         # If we can't install SIGHUP, there is no reason (yet) to do graceful
         # shutdown.
-        return
-    old = signal.signal(signal.SIGHUP, _sighup_handler)
-    _setup_on_hangup_dict()
-    return old
+        old_signal = None
+    else:
+        old_signal = signal.signal(signal.SIGHUP, _sighup_handler)
+    old_dict = _setup_on_hangup_dict()
+    return old_signal, old_dict
 
 
 def _setup_on_hangup_dict():
@@ -75,6 +76,15 @@
     return old
 
 
+def restore_sighup_handler(orig):
+    """Pass in the returned value from install_sighup_handler to reset."""
+    global _on_sighup
+    old_signal, old_dict = orig
+    if old_signal is not None:
+        signal.signal(signal.SIGHUP, old_signal)
+    _on_sighup = old_dict
+
+
 # TODO: Should these be single-use callables? Meaning that once we've triggered
 #       SIGHUP and called them, they should auto-remove themselves? I don't
 #       think so. Callers need to clean up during shutdown anyway, so that we

=== modified file 'bzrlib/tests/test_smart_signals.py'
--- a/bzrlib/tests/test_smart_signals.py	2011-09-23 11:43:51 +0000
+++ b/bzrlib/tests/test_smart_signals.py	2011-09-23 18:11:28 +0000
@@ -131,14 +131,13 @@
         self.assertEqual('', log)
 
     def test_install_sighup_handler(self):
-        if getattr(signal, 'SIGHUP', None) is None:
-            raise tests.TestNotApplicable('No SIGHUP to handle on this'
-                ' platform (%s)' % (sys.platform,))
         # install_sighup_handler should set up a signal handler for SIGHUP, as
         # well as the signals._on_sighup dict.
-        # TODO: Windows testing
         signals._on_sighup = None
         orig = signals.install_sighup_handler()
-        old = signal.signal(SIGHUP, orig)
+        if getattr(signal, 'SIGHUP', None) is not None:
+            cur = signal.getsignal(SIGHUP, orig)
+            self.assertEqual(signals._sighup_handler, cur)
         self.assertIsNot(None, signals._on_sighup)
-        self.assertEqual(signals._sighup_handler, old)
+        signals.restore_sighup_handler(orig)
+        self.assertIs(None, signals._on_sighup)