Merge lp:~jr/bzr/274578-smart-server-hookable-error-logging into lp:bzr

[Jonathan Riddell]

New hook server_exception in bzrlib.smart.server to catch any exception caused while running bzr serve. (Jonathan Riddell, #274578)

Add "server_exception" hook for bug 61728. When an exception occurs it calls the hook with sys.exc_info(). If the hook returns false or there is no hook is raises the exception as normal.

[Vincent Ladeuil (vila)]

Looks good, thanks !

One small nit:

70 + args = []
71 + out, err = self.run_bzr_serve_then_func(args, retcode=3)

This checks that the server dies when no hook is installed (before testing the hook itself).

It would be even better to do that in a separate test to get a better defect localization (see http://xunitpatterns.com/Goals%20of%20Test%20Automation.html).

You also need to add a news entry (make sure to remerge trunk so you get the right section in the release-notes file).

BB:tweak ;)

review: Needs Fixing

[Jonathan Riddell (jr)]

sent to pqm by email

[Jonathan Riddell (jr)]

sent to pqm by email

=== modified file 'bzrlib/smart/server.py'
--- bzrlib/smart/server.py	2011-03-30 11:45:54 +0000
+++ bzrlib/smart/server.py	2011-05-27 13:15:43 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2010 Canonical Ltd
+# Copyright (C) 2006-2011 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -254,6 +254,11 @@
             "Called by the bzr server when it stops serving a directory. "
             "server_stopped is called with the same parameters as the "
             "server_started hook: (backing_urls, public_url).", (0, 16))
+        self.add_hook('server_exception',
+            "Called by the bzr server when an exception occurs. "
+            "server_exception is called with the sys.exc_info() tuple "
+            "return true for the hook if the exception has been handled, "
+            "in which case the server will exit normally.", (2, 4))
 
 SmartTCPServer.hooks = SmartServerHooks()
 
@@ -373,7 +378,6 @@
         for cleanup in reversed(self.cleanups):
             cleanup()
 
-
 def serve_bzr(transport, host=None, port=None, inet=False):
     """This is the default implementation of 'bzr serve'.
     
@@ -385,6 +389,11 @@
     try:
         bzr_server.set_up(transport, host, port, inet)
         bzr_server.smart_server.serve()
+    except:
+        hook_caught_exception = False
+        for hook in SmartTCPServer.hooks['server_exception']:
+            hook_caught_exception = hook(sys.exc_info())
+        if not hook_caught_exception:
+            raise
     finally:
         bzr_server.tear_down()
-

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2011-05-26 08:05:45 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2011-05-27 13:15:43 +0000
@@ -81,7 +81,7 @@
             'run_bzr_serve_then_func hook')
         # start a TCP server
         try:
-            out, err = self.run_bzr(['serve'] + list(serve_args))
+            out, err = self.run_bzr(['serve'] + list(serve_args), retcode=retcode)
         except KeyboardInterrupt, e:
             out, err = e.args
         return out, err
@@ -93,6 +93,24 @@
         super(TestBzrServe, self).setUp()
         self.disable_missing_extensions_warning()
 
+    def test_server_exception_with_hook(self):
+        """test exception hook works to catch exceptions from server"""
+        def hook(exception):
+            from bzrlib.trace import note
+            note("catching exception")
+            return True
+        SmartTCPServer.hooks.install_named_hook(
+            'server_exception', hook,
+            'test_server_except_hook hook')
+        args = []
+        out, err = self.run_bzr_serve_then_func(args, retcode=0)
+        self.assertEqual('listening on port: 4155\ncatching exception\n', err)
+
+    def test_server_exception_no_hook(self):
+        """test exception without hook returns error"""
+        args = []
+        out, err = self.run_bzr_serve_then_func(args, retcode=3)
+
     def assertInetServerShutsdownCleanly(self, process):
         """Shutdown the server process looking for errors."""
         # Shutdown the server: the server should shut down when it cannot read

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-27 11:05:47 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-27 13:15:43 +0000
@@ -20,6 +20,10 @@
 
 .. New commands, options, etc that users may wish to try out.
 
+* New hook server_exception in bzrlib.smart.server to catch any
+  exception caused while running bzr serve.  (Jonathan Riddell,
+  #274578)
+
 Improvements
 ************