Issue #23430: Stop socketserver from catching SystemExit etc from handlers

Also make handle_error() consistently output to stderr, and fix the
documentation.

5 files changed, 131 insertions, 15 deletions

diff --git a/Doc/library/socketserver.rst b/Doc/library/socketserver.rst
index f0cfc80bf3..aaaa61e9d9 100644
--- a/Doc/library/socketserver.rst
+++ b/Doc/library/socketserver.rst
@@ -304,7 +304,11 @@ Server Objects
       This function is called if the :meth:`~BaseRequestHandler.handle`
       method of a :attr:`RequestHandlerClass` instance raises
       an exception.  The default action is to print the traceback to
-      standard output and continue handling further requests.
+      standard error and continue handling further requests.
+
+      .. versionchanged:: 3.6
+         Now only called for exceptions derived from the :exc:`Exception`
+         class.
 
 
    .. method:: handle_timeout()
diff --git a/Doc/whatsnew/3.6.rst b/Doc/whatsnew/3.6.rst
index 8b879deb5d..5c02b7df8b 100644
--- a/Doc/whatsnew/3.6.rst
+++ b/Doc/whatsnew/3.6.rst
@@ -334,7 +334,16 @@ Changes in the Python API
 
 * When a relative import is performed and no parent package is known, then
   :exc:`ImportError` will be raised. Previously, :exc:`SystemError` could be
-  raised. (Contribute by Brett Cannon in :issue:`18018`.)
+  raised. (Contributed by Brett Cannon in :issue:`18018`.)
+
+* Servers based on the :mod:`socketserver` module, including those
+  defined in :mod:`http.server`, :mod:`xmlrpc.server` and
+  :mod:`wsgiref.simple_server`, now only catch exceptions derived
+  from :exc:`Exception`. Therefore if a request handler raises
+  an exception like :exc:`SystemExit` or :exc:`KeyboardInterrupt`,
+  :meth:`~socketserver.BaseServer.handle_error` is no longer called, and
+  the exception will stop a single-threaded server. (Contributed by
+  Martin Panter in :issue:`23430`.)
 
 
 Changes in the C API
diff --git a/Lib/socketserver.py b/Lib/socketserver.py
index 1524d1634e..6468999188 100644
--- a/Lib/socketserver.py
+++ b/Lib/socketserver.py
@@ -132,6 +132,7 @@ import socket
 import selectors
 import os
 import errno
+import sys
 try:
     import threading
 except ImportError:
@@ -316,9 +317,12 @@ class BaseServer:
         if self.verify_request(request, client_address):
             try:
                 self.process_request(request, client_address)
-            except:
+            except Exception:
                 self.handle_error(request, client_address)
                 self.shutdown_request(request)
+            except:
+                self.shutdown_request(request)
+                raise
         else:
             self.shutdown_request(request)
 
@@ -372,12 +376,12 @@ class BaseServer:
         The default is to print a traceback and continue.
 
         """
-        print('-'*40)
-        print('Exception happened during processing of request from', end=' ')
-        print(client_address)
+        print('-'*40, file=sys.stderr)
+        print('Exception happened during processing of request from',
+            client_address, file=sys.stderr)
         import traceback
-        traceback.print_exc() # XXX But this goes to stderr!
-        print('-'*40)
+        traceback.print_exc()
+        print('-'*40, file=sys.stderr)
 
 
 class TCPServer(BaseServer):
@@ -601,16 +605,17 @@ class ForkingMixIn:
         else:
             # Child process.
             # This must never return, hence os._exit()!
+            status = 1
             try:
                 self.finish_request(request, client_address)
-                self.shutdown_request(request)
-                os._exit(0)
-            except:
+                status = 0
+            except Exception:
+                self.handle_error(request, client_address)
+            finally:
                 try:
-                    self.handle_error(request, client_address)
                     self.shutdown_request(request)
                 finally:
-                    os._exit(1)
+                    os._exit(status)
 
 
 class ThreadingMixIn:
@@ -628,9 +633,9 @@ class ThreadingMixIn:
         """
         try:
             self.finish_request(request, client_address)
-            self.shutdown_request(request)
-        except:
+        except Exception:
             self.handle_error(request, client_address)
+        finally:
             self.shutdown_request(request)
 
     def process_request(self, request, client_address):
diff --git a/Lib/test/test_socketserver.py b/Lib/test/test_socketserver.py
index dc23210378..bff0ff4b49 100644
--- a/Lib/test/test_socketserver.py
+++ b/Lib/test/test_socketserver.py
@@ -58,6 +58,7 @@ if HAVE_UNIX_SOCKETS:
 
 @contextlib.contextmanager
 def simple_subprocess(testcase):
+    """Tests that a custom child process is not waited on (Issue 1540386)"""
     pid = os.fork()
     if pid == 0:
         # Don't raise an exception; it would be caught by the test harness.
@@ -281,6 +282,97 @@ class SocketServerTest(unittest.TestCase):
                                        socketserver.StreamRequestHandler)
 
 
+class ErrorHandlerTest(unittest.TestCase):
+    """Test that the servers pass normal exceptions from the handler to
+    handle_error(), and that exiting exceptions like SystemExit and
+    KeyboardInterrupt are not passed."""
+
+    def tearDown(self):
+        test.support.unlink(test.support.TESTFN)
+
+    def test_sync_handled(self):
+        BaseErrorTestServer(ValueError)
+        self.check_result(handled=True)
+
+    def test_sync_not_handled(self):
+        with self.assertRaises(SystemExit):
+            BaseErrorTestServer(SystemExit)
+        self.check_result(handled=False)
+
+    @unittest.skipUnless(threading, 'Threading required for this test.')
+    def test_threading_handled(self):
+        ThreadingErrorTestServer(ValueError)
+        self.check_result(handled=True)
+
+    @unittest.skipUnless(threading, 'Threading required for this test.')
+    def test_threading_not_handled(self):
+        ThreadingErrorTestServer(SystemExit)
+        self.check_result(handled=False)
+
+    @requires_forking
+    def test_forking_handled(self):
+        ForkingErrorTestServer(ValueError)
+        self.check_result(handled=True)
+
+    @requires_forking
+    def test_forking_not_handled(self):
+        ForkingErrorTestServer(SystemExit)
+        self.check_result(handled=False)
+
+    def check_result(self, handled):
+        with open(test.support.TESTFN) as log:
+            expected = 'Handler called\n' + 'Error handled\n' * handled
+            self.assertEqual(log.read(), expected)
+
+
+class BaseErrorTestServer(socketserver.TCPServer):
+    def __init__(self, exception):
+        self.exception = exception
+        super().__init__((HOST, 0), BadHandler)
+        with socket.create_connection(self.server_address):
+            pass
+        try:
+            self.handle_request()
+        finally:
+            self.server_close()
+        self.wait_done()
+
+    def handle_error(self, request, client_address):
+        with open(test.support.TESTFN, 'a') as log:
+            log.write('Error handled\n')
+
+    def wait_done(self):
+        pass
+
+
+class BadHandler(socketserver.BaseRequestHandler):
+    def handle(self):
+        with open(test.support.TESTFN, 'a') as log:
+            log.write('Handler called\n')
+        raise self.server.exception('Test error')
+
+
+class ThreadingErrorTestServer(socketserver.ThreadingMixIn,
+        BaseErrorTestServer):
+    def __init__(self, *pos, **kw):
+        self.done = threading.Event()
+        super().__init__(*pos, **kw)
+
+    def shutdown_request(self, *pos, **kw):
+        super().shutdown_request(*pos, **kw)
+        self.done.set()
+
+    def wait_done(self):
+        self.done.wait()
+
+
+class ForkingErrorTestServer(socketserver.ForkingMixIn, BaseErrorTestServer):
+    def wait_done(self):
+        [child] = self.active_children
+        os.waitpid(child, 0)
+        self.active_children.clear()
+
+
 class MiscTestCase(unittest.TestCase):
 
     def test_all(self):
diff --git a/Misc/NEWS b/Misc/NEWS
index 6c1e83f4c6..4043e0139e 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -196,6 +196,12 @@ Issue #26186: Remove an invalid type check in importlib.util.LazyLoader.
   the connected socket) when verify_request() returns false.  Patch by Aviv
   Palivoda.
 
+- Issue #23430: Change the socketserver module to only catch exceptions
+  raised from a request handler that are derived from Exception (instead of
+  BaseException).  Therefore SystemExit and KeyboardInterrupt no longer
+  trigger the handle_error() method, and will now to stop a single-threaded
+  server.
+
 - Issue #25939: On Windows open the cert store readonly in ssl.enum_certificates.
 
 - Issue #25995: os.walk() no longer uses FDs proportional to the tree depth.