diff --git a/Doc/library/socketserver.rst b/Doc/library/socketserver.rst index f0cfc80bf30..aaaa61e9d97 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 8b879deb5dd..5c02b7df8b5 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 1524d1634eb..64689991884 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 dc23210378f..bff0ff4b49a 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 6c1e83f4c6f..4043e0139e4 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.