From 431b540bf79f0982559b1b0e420b1b085f667bb7 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 27 May 2019 14:45:12 +0200 Subject: [PATCH] bpo-32528: Make asyncio.CancelledError a BaseException. (GH-13528) This will address the common mistake many asyncio users make: an "except Exception" clause breaking Tasks cancellation. In addition to this change, we stop inheriting asyncio.TimeoutError and asyncio.InvalidStateError from their concurrent.futures.* counterparts. There's no point for these exceptions to share the inheritance chain. In 3.9 we'll focus on implementing supervisors and cancel scopes, which should allow better handling of all exceptions, including SystemExit and KeyboardInterrupt --- Lib/asyncio/base_events.py | 16 ++-- Lib/asyncio/base_subprocess.py | 4 +- Lib/asyncio/events.py | 4 +- Lib/asyncio/exceptions.py | 9 +-- Lib/asyncio/proactor_events.py | 12 ++- Lib/asyncio/selector_events.py | 76 ++++++++++++++----- Lib/asyncio/sslproto.py | 16 +++- Lib/asyncio/staggered.py | 4 +- Lib/asyncio/tasks.py | 12 +-- Lib/asyncio/transports.py | 8 +- Lib/asyncio/unix_events.py | 20 +++-- Lib/asyncio/windows_events.py | 4 +- Lib/test/test_asyncio/test_base_events.py | 6 +- Lib/test/test_asyncio/test_tasks.py | 4 +- .../2019-05-23-17-37-22.bpo-32528.sGnkcl.rst | 8 ++ Modules/_asynciomodule.c | 11 +-- 16 files changed, 147 insertions(+), 67 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-05-23-17-37-22.bpo-32528.sGnkcl.rst diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index de9fa4f4f7f..63b072b851e 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -186,7 +186,7 @@ def _interleave_addrinfos(addrinfos, first_address_family_count=1): def _run_until_complete_cb(fut): if not fut.cancelled(): exc = fut.exception() - if isinstance(exc, BaseException) and not isinstance(exc, Exception): + if isinstance(exc, (SystemExit, KeyboardInterrupt)): # Issue #22429: run_forever() already finished, no need to # stop it. return @@ -1196,7 +1196,7 @@ class BaseEventLoop(events.AbstractEventLoop): try: await waiter - except Exception: + except BaseException: transport.close() conmade_cb.cancel() resume_cb.cancel() @@ -1710,7 +1710,9 @@ class BaseEventLoop(events.AbstractEventLoop): if self._exception_handler is None: try: self.default_exception_handler(context) - except Exception: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException: # Second protection layer for unexpected errors # in the default implementation, as well as for subclassed # event loops with overloaded "default_exception_handler". @@ -1719,7 +1721,9 @@ class BaseEventLoop(events.AbstractEventLoop): else: try: self._exception_handler(self, context) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: # Exception in the user set custom exception handler. try: # Let's try default handler. @@ -1728,7 +1732,9 @@ class BaseEventLoop(events.AbstractEventLoop): 'exception': exc, 'context': context, }) - except Exception: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException: # Guard 'default_exception_handler' in case it is # overloaded. logger.error('Exception in default exception handler ' diff --git a/Lib/asyncio/base_subprocess.py b/Lib/asyncio/base_subprocess.py index f503f78fdda..14d50519228 100644 --- a/Lib/asyncio/base_subprocess.py +++ b/Lib/asyncio/base_subprocess.py @@ -182,7 +182,9 @@ class BaseSubprocessTransport(transports.SubprocessTransport): for callback, data in self._pending_calls: loop.call_soon(callback, *data) self._pending_calls = None - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: if waiter is not None and not waiter.cancelled(): waiter.set_exception(exc) else: diff --git a/Lib/asyncio/events.py b/Lib/asyncio/events.py index 9a923514db0..d381b1c5962 100644 --- a/Lib/asyncio/events.py +++ b/Lib/asyncio/events.py @@ -79,7 +79,9 @@ class Handle: def _run(self): try: self._context.run(self._callback, *self._args) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: cb = format_helpers._format_callback_source( self._callback, self._args) msg = f'Exception in callback {cb}' diff --git a/Lib/asyncio/exceptions.py b/Lib/asyncio/exceptions.py index cac31a54d25..e03602ef576 100644 --- a/Lib/asyncio/exceptions.py +++ b/Lib/asyncio/exceptions.py @@ -5,19 +5,16 @@ __all__ = ('CancelledError', 'InvalidStateError', 'TimeoutError', 'IncompleteReadError', 'LimitOverrunError', 'SendfileNotAvailableError') -import concurrent.futures -from . import base_futures - -class CancelledError(concurrent.futures.CancelledError): +class CancelledError(BaseException): """The Future or Task was cancelled.""" -class TimeoutError(concurrent.futures.TimeoutError): +class TimeoutError(Exception): """The operation exceeded the given deadline.""" -class InvalidStateError(concurrent.futures.InvalidStateError): +class InvalidStateError(Exception): """The operation is not allowed in this state.""" diff --git a/Lib/asyncio/proactor_events.py b/Lib/asyncio/proactor_events.py index a849be1cc14..7dfe29579a0 100644 --- a/Lib/asyncio/proactor_events.py +++ b/Lib/asyncio/proactor_events.py @@ -212,7 +212,9 @@ class _ProactorReadPipeTransport(_ProactorBasePipeTransport, try: keep_open = self._protocol.eof_received() - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal error: protocol.eof_received() call failed.') return @@ -235,7 +237,9 @@ class _ProactorReadPipeTransport(_ProactorBasePipeTransport, if isinstance(self._protocol, protocols.BufferedProtocol): try: protocols._feed_data_to_buffered_proto(self._protocol, data) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error(exc, 'Fatal error: protocol.buffer_updated() ' 'call failed.') @@ -625,7 +629,9 @@ class BaseProactorEventLoop(base_events.BaseEventLoop): except exceptions.CancelledError: # _close_self_pipe() has been called, stop waiting for data return - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self.call_exception_handler({ 'message': 'Error on reading from the event loop self pipe', 'exception': exc, diff --git a/Lib/asyncio/selector_events.py b/Lib/asyncio/selector_events.py index 6461d307763..f5f43a9bfef 100644 --- a/Lib/asyncio/selector_events.py +++ b/Lib/asyncio/selector_events.py @@ -208,12 +208,14 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): try: await waiter - except: + except BaseException: transport.close() raise + # It's now up to the protocol to handle the connection. - # It's now up to the protocol to handle the connection. - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: if self._debug: context = { 'message': @@ -370,7 +372,9 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): data = sock.recv(n) except (BlockingIOError, InterruptedError): return # try again next time - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: fut.set_exception(exc) else: fut.set_result(data) @@ -404,7 +408,9 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): nbytes = sock.recv_into(buf) except (BlockingIOError, InterruptedError): return # try again next time - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: fut.set_exception(exc) else: fut.set_result(nbytes) @@ -447,7 +453,9 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): n = sock.send(view[start:]) except (BlockingIOError, InterruptedError): return - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: fut.set_exception(exc) return @@ -487,7 +495,9 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): fut.add_done_callback( functools.partial(self._sock_write_done, fd)) self.add_writer(fd, self._sock_connect_cb, fut, sock, address) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: fut.set_exception(exc) else: fut.set_result(None) @@ -507,7 +517,9 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): except (BlockingIOError, InterruptedError): # socket is still registered, the callback will be retried later pass - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: fut.set_exception(exc) else: fut.set_result(None) @@ -537,7 +549,9 @@ class BaseSelectorEventLoop(base_events.BaseEventLoop): conn.setblocking(False) except (BlockingIOError, InterruptedError): self.add_reader(fd, self._sock_accept, fut, True, sock) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: fut.set_exception(exc) else: fut.set_result((conn, address)) @@ -785,7 +799,9 @@ class _SelectorSocketTransport(_SelectorTransport): buf = self._protocol.get_buffer(-1) if not len(buf): raise RuntimeError('get_buffer() returned an empty buffer') - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal error: protocol.get_buffer() call failed.') return @@ -794,7 +810,9 @@ class _SelectorSocketTransport(_SelectorTransport): nbytes = self._sock.recv_into(buf) except (BlockingIOError, InterruptedError): return - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error(exc, 'Fatal read error on socket transport') return @@ -804,7 +822,9 @@ class _SelectorSocketTransport(_SelectorTransport): try: self._protocol.buffer_updated(nbytes) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal error: protocol.buffer_updated() call failed.') @@ -815,7 +835,9 @@ class _SelectorSocketTransport(_SelectorTransport): data = self._sock.recv(self.max_size) except (BlockingIOError, InterruptedError): return - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error(exc, 'Fatal read error on socket transport') return @@ -825,7 +847,9 @@ class _SelectorSocketTransport(_SelectorTransport): try: self._protocol.data_received(data) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal error: protocol.data_received() call failed.') @@ -835,7 +859,9 @@ class _SelectorSocketTransport(_SelectorTransport): try: keep_open = self._protocol.eof_received() - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal error: protocol.eof_received() call failed.') return @@ -871,7 +897,9 @@ class _SelectorSocketTransport(_SelectorTransport): n = self._sock.send(data) except (BlockingIOError, InterruptedError): pass - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error(exc, 'Fatal write error on socket transport') return else: @@ -894,7 +922,9 @@ class _SelectorSocketTransport(_SelectorTransport): n = self._sock.send(self._buffer) except (BlockingIOError, InterruptedError): pass - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._loop._remove_writer(self._sock_fd) self._buffer.clear() self._fatal_error(exc, 'Fatal write error on socket transport') @@ -970,7 +1000,9 @@ class _SelectorDatagramTransport(_SelectorTransport): pass except OSError as exc: self._protocol.error_received(exc) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error(exc, 'Fatal read error on datagram transport') else: self._protocol.datagram_received(data, addr) @@ -1007,7 +1039,9 @@ class _SelectorDatagramTransport(_SelectorTransport): except OSError as exc: self._protocol.error_received(exc) return - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal write error on datagram transport') return @@ -1030,7 +1064,9 @@ class _SelectorDatagramTransport(_SelectorTransport): except OSError as exc: self._protocol.error_received(exc) return - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._fatal_error( exc, 'Fatal write error on datagram transport') return diff --git a/Lib/asyncio/sslproto.py b/Lib/asyncio/sslproto.py index 97a6fc66a92..8546985fe63 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -527,7 +527,9 @@ class SSLProtocol(protocols.Protocol): try: ssldata, appdata = self._sslpipe.feed_ssldata(data) - except Exception as e: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as e: self._fatal_error(e, 'SSL error in data received') return @@ -542,7 +544,9 @@ class SSLProtocol(protocols.Protocol): self._app_protocol, chunk) else: self._app_protocol.data_received(chunk) - except Exception as ex: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as ex: self._fatal_error( ex, 'application protocol failed to receive SSL data') return @@ -628,7 +632,9 @@ class SSLProtocol(protocols.Protocol): raise handshake_exc peercert = sslobj.getpeercert() - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: if isinstance(exc, ssl.CertificateError): msg = 'SSL handshake failed on verifying the certificate' else: @@ -691,7 +697,9 @@ class SSLProtocol(protocols.Protocol): # delete it and reduce the outstanding buffer size. del self._write_backlog[0] self._write_buffer_size -= len(data) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: if self._in_handshake: # Exceptions will be re-raised in _on_handshake_complete. self._on_handshake_complete(exc) diff --git a/Lib/asyncio/staggered.py b/Lib/asyncio/staggered.py index feec681b437..27c665a9910 100644 --- a/Lib/asyncio/staggered.py +++ b/Lib/asyncio/staggered.py @@ -105,7 +105,9 @@ async def staggered_race( try: result = await coro_fn() - except Exception as e: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as e: exceptions[this_index] = e this_failed.set() # Kickstart the next coroutine else: diff --git a/Lib/asyncio/tasks.py b/Lib/asyncio/tasks.py index 1dc595298c5..78e76003b3a 100644 --- a/Lib/asyncio/tasks.py +++ b/Lib/asyncio/tasks.py @@ -260,11 +260,11 @@ class Task(futures._PyFuture): # Inherit Python Task implementation super().set_result(exc.value) except exceptions.CancelledError: super().cancel() # I.e., Future.cancel(self). - except Exception as exc: - super().set_exception(exc) - except BaseException as exc: + except (KeyboardInterrupt, SystemExit) as exc: super().set_exception(exc) raise + except BaseException as exc: + super().set_exception(exc) else: blocking = getattr(result, '_asyncio_future_blocking', None) if blocking is not None: @@ -318,7 +318,7 @@ class Task(futures._PyFuture): # Inherit Python Task implementation def __wakeup(self, future): try: future.result() - except Exception as exc: + except BaseException as exc: # This may also be a cancellation. self.__step(exc) else: @@ -858,7 +858,9 @@ def run_coroutine_threadsafe(coro, loop): def callback(): try: futures._chain_future(ensure_future(coro, loop=loop), future) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: if future.set_running_or_notify_cancel(): future.set_exception(exc) raise diff --git a/Lib/asyncio/transports.py b/Lib/asyncio/transports.py index 233bbb53cb6..47b37fa9b7f 100644 --- a/Lib/asyncio/transports.py +++ b/Lib/asyncio/transports.py @@ -262,7 +262,9 @@ class _FlowControlMixin(Transport): self._protocol_paused = True try: self._protocol.pause_writing() - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._loop.call_exception_handler({ 'message': 'protocol.pause_writing() failed', 'exception': exc, @@ -276,7 +278,9 @@ class _FlowControlMixin(Transport): self._protocol_paused = False try: self._protocol.resume_writing() - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._loop.call_exception_handler({ 'message': 'protocol.resume_writing() failed', 'exception': exc, diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index 1aa3b396086..81d10b190cc 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -194,7 +194,9 @@ class _UnixSelectorEventLoop(selector_events.BaseSelectorEventLoop): self._child_watcher_callback, transp) try: await waiter - except Exception: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException: transp.close() await transp._wait() raise @@ -390,7 +392,9 @@ class _UnixSelectorEventLoop(selector_events.BaseSelectorEventLoop): else: self._sock_sendfile_update_filepos(fileno, offset, total_sent) fut.set_exception(exc) - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._sock_sendfile_update_filepos(fileno, offset, total_sent) fut.set_exception(exc) else: @@ -641,7 +645,9 @@ class _UnixWritePipeTransport(transports._FlowControlMixin, n = os.write(self._fileno, data) except (BlockingIOError, InterruptedError): n = 0 - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._conn_lost += 1 self._fatal_error(exc, 'Fatal write error on pipe transport') return @@ -661,7 +667,9 @@ class _UnixWritePipeTransport(transports._FlowControlMixin, n = os.write(self._fileno, self._buffer) except (BlockingIOError, InterruptedError): pass - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: self._buffer.clear() self._conn_lost += 1 # Remove writer here, _fatal_error() doesn't it @@ -879,7 +887,9 @@ class BaseChildWatcher(AbstractChildWatcher): def _sig_chld(self): try: self._do_waitpid_all() - except Exception as exc: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException as exc: # self._loop should always be available here # as '_sig_chld' is added as a signal handler # in 'attach_loop' diff --git a/Lib/asyncio/windows_events.py b/Lib/asyncio/windows_events.py index 29750f18d80..b5b2e24c5ba 100644 --- a/Lib/asyncio/windows_events.py +++ b/Lib/asyncio/windows_events.py @@ -388,7 +388,9 @@ class ProactorEventLoop(proactor_events.BaseProactorEventLoop): **kwargs) try: await waiter - except Exception: + except (SystemExit, KeyboardInterrupt): + raise + except BaseException: transp.close() await transp._wait() raise diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index f068fc781f5..31018c5c563 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -476,7 +476,7 @@ class BaseEventLoopTests(test_utils.TestCase): other_loop.run_until_complete, task) def test_run_until_complete_loop_orphan_future_close_loop(self): - class ShowStopper(BaseException): + class ShowStopper(SystemExit): pass async def foo(delay): @@ -487,10 +487,8 @@ class BaseEventLoopTests(test_utils.TestCase): self.loop._process_events = mock.Mock() self.loop.call_soon(throw) - try: + with self.assertRaises(ShowStopper): self.loop.run_until_complete(foo(0.1)) - except ShowStopper: - pass # This call fails if run_until_complete does not clean up # done-callback for the previous future. diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index 1c1f912ff8a..114dd76687c 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -1527,7 +1527,7 @@ class BaseTaskTests: async def sleeper(): await asyncio.sleep(10) - base_exc = BaseException() + base_exc = SystemExit() async def notmutch(): try: @@ -1541,7 +1541,7 @@ class BaseTaskTests: task.cancel() self.assertFalse(task.done()) - self.assertRaises(BaseException, test_utils.run_briefly, loop) + self.assertRaises(SystemExit, test_utils.run_briefly, loop) self.assertTrue(task.done()) self.assertFalse(task.cancelled()) diff --git a/Misc/NEWS.d/next/Library/2019-05-23-17-37-22.bpo-32528.sGnkcl.rst b/Misc/NEWS.d/next/Library/2019-05-23-17-37-22.bpo-32528.sGnkcl.rst new file mode 100644 index 00000000000..375f426025d --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-05-23-17-37-22.bpo-32528.sGnkcl.rst @@ -0,0 +1,8 @@ +Make asyncio.CancelledError a BaseException. + +This will address the common mistake many asyncio users make: an "except +Exception" clause breaking Tasks cancellation. + +In addition to this change, we stop inheriting asyncio.TimeoutError and +asyncio.InvalidStateError from their concurrent.futures.* counterparts. +There's no point for these exceptions to share the inheritance chain. diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index f9037c279ac..ac15d0169b8 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2672,8 +2672,10 @@ set_exception: assert(o == Py_None); Py_DECREF(o); - if (!PyErr_GivenExceptionMatches(et, PyExc_Exception)) { - /* We've got a BaseException; re-raise it */ + if (PyErr_GivenExceptionMatches(et, PyExc_KeyboardInterrupt) || + PyErr_GivenExceptionMatches(et, PyExc_SystemExit)) + { + /* We've got a KeyboardInterrupt or a SystemError; re-raise it */ PyErr_Restore(et, ev, tb); goto fail; } @@ -2950,11 +2952,6 @@ task_wakeup(TaskObj *task, PyObject *o) } PyErr_Fetch(&et, &ev, &tb); - if (!PyErr_GivenExceptionMatches(et, PyExc_Exception)) { - /* We've got a BaseException; re-raise it */ - PyErr_Restore(et, ev, tb); - return NULL; - } if (!ev || !PyObject_TypeCheck(ev, (PyTypeObject *) et)) { PyErr_NormalizeException(&et, &ev, &tb); }