From 47e064f3ffd311be95b46a7b283100e0bd9dd225 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Thu, 23 May 2019 17:37:29 -0400 Subject: [PATCH 1/2] bpo-32528: 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. --- 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 | 10 ++- Lib/asyncio/transports.py | 8 +- Lib/asyncio/unix_events.py | 20 +++-- Lib/asyncio/windows_events.py | 4 +- Lib/test/test_asyncio/test_tasks.py | 4 +- .../2019-05-23-17-37-22.bpo-32528.sGnkcl.rst | 8 ++ Modules/_asynciomodule.c | 11 +-- 15 files changed, 144 insertions(+), 62 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 de9fa4f4f7f3b6..63b072b851e3ea 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 @@ async def start_tls(self, transport, protocol, sslcontext, *, try: await waiter - except Exception: + except BaseException: transport.close() conmade_cb.cancel() resume_cb.cancel() @@ -1710,7 +1710,9 @@ def call_exception_handler(self, context): 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 @@ def call_exception_handler(self, context): 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 @@ def call_exception_handler(self, context): '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 f503f78fdda349..14d50519228814 100644 --- a/Lib/asyncio/base_subprocess.py +++ b/Lib/asyncio/base_subprocess.py @@ -182,7 +182,9 @@ async def _connect_pipes(self, waiter): 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 9a923514db0993..d381b1c596239c 100644 --- a/Lib/asyncio/events.py +++ b/Lib/asyncio/events.py @@ -79,7 +79,9 @@ def cancelled(self): 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 cac31a54d2531a..e03602ef576234 100644 --- a/Lib/asyncio/exceptions.py +++ b/Lib/asyncio/exceptions.py @@ -5,19 +5,16 @@ '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 a849be1cc1479b..7dfe29579a0dee 100644 --- a/Lib/asyncio/proactor_events.py +++ b/Lib/asyncio/proactor_events.py @@ -212,7 +212,9 @@ def _eof_received(self): 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 @@ def _data_received(self, data): 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 @@ def _loop_self_reading(self, f=None): 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 6461d3077633d0..f5f43a9bfef33a 100644 --- a/Lib/asyncio/selector_events.py +++ b/Lib/asyncio/selector_events.py @@ -208,12 +208,14 @@ async def _accept_connection2( 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 @@ def _sock_recv(self, fut, sock, n): 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 @@ def _sock_recv_into(self, fut, sock, buf): 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 @@ def _sock_sendall(self, fut, sock, view, pos): 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 @@ def _sock_connect(self, fut, sock, address): 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 @@ def _sock_connect_cb(self, fut, sock, address): 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 @@ def _sock_accept(self, fut, registered, sock): 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 @@ def _read_ready__get_buffer(self): 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 @@ def _read_ready__get_buffer(self): 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 @@ def _read_ready__get_buffer(self): 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 @@ def _read_ready__data_received(self): 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 @@ def _read_ready__data_received(self): 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 @@ def _read_ready__on_eof(self): 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 @@ def write(self, data): 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 @@ def _write_ready(self): 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 @@ def _read_ready(self): 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 @@ def sendto(self, data, addr=None): 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 @@ def _sendto_ready(self): 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 97a6fc66a92735..8546985fe63fc6 100644 --- a/Lib/asyncio/sslproto.py +++ b/Lib/asyncio/sslproto.py @@ -527,7 +527,9 @@ def data_received(self, data): 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 @@ def data_received(self, data): 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 @@ def _on_handshake_complete(self, handshake_exc): 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 @@ def _process_write_backlog(self): # 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 feec681b4371bf..27c665a9910ab2 100644 --- a/Lib/asyncio/staggered.py +++ b/Lib/asyncio/staggered.py @@ -105,7 +105,9 @@ async def run_one_coro( 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 1dc595298c556d..78e76003b3ac22 100644 --- a/Lib/asyncio/tasks.py +++ b/Lib/asyncio/tasks.py @@ -260,11 +260,11 @@ def __step(self, exc=None): super().set_result(exc.value) except exceptions.CancelledError: super().cancel() # I.e., Future.cancel(self). - except Exception as exc: + except (KeyboardInterrupt, SystemExit) as exc: super().set_exception(exc) + raise except BaseException as exc: super().set_exception(exc) - raise else: blocking = getattr(result, '_asyncio_future_blocking', None) if blocking is not None: @@ -318,7 +318,7 @@ def __step(self, exc=None): 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 233bbb53cb6a3f..47b37fa9b7f0f6 100644 --- a/Lib/asyncio/transports.py +++ b/Lib/asyncio/transports.py @@ -262,7 +262,9 @@ def _maybe_pause_protocol(self): 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 @@ def _maybe_resume_protocol(self): 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 1aa3b396086c59..81d10b190cc6de 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -194,7 +194,9 @@ async def _make_subprocess_transport(self, protocol, args, shell, 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 @@ def _sock_sendfile_native_impl(self, fut, registered_fd, sock, fileno, 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 @@ def write(self, data): 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 @@ def _write_ready(self): 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 @@ def attach_loop(self, loop): 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 29750f18d80c46..b5b2e24c5ba4f3 100644 --- a/Lib/asyncio/windows_events.py +++ b/Lib/asyncio/windows_events.py @@ -388,7 +388,9 @@ async def _make_subprocess_transport(self, protocol, args, shell, **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_tasks.py b/Lib/test/test_asyncio/test_tasks.py index 1c1f912ff8af5c..114dd76687cd70 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -1527,7 +1527,7 @@ def gen(): async def sleeper(): await asyncio.sleep(10) - base_exc = BaseException() + base_exc = SystemExit() async def notmutch(): try: @@ -1541,7 +1541,7 @@ async def notmutch(): 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 00000000000000..375f426025d325 --- /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 f9037c279ac9c3..ac15d0169b8158 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2672,8 +2672,10 @@ task_step_impl(TaskObj *task, PyObject *exc) 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); } From 04bd65ef1b8a0de7e0aa41e0d4dc5992da6e1a07 Mon Sep 17 00:00:00 2001 From: Yury Selivanov Date: Mon, 27 May 2019 14:23:21 +0200 Subject: [PATCH 2/2] Fix test_run_until_complete_loop_orphan_future_close_loop --- Lib/test/test_asyncio/test_base_events.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py index f068fc781f5d7c..31018c5c563637 100644 --- a/Lib/test/test_asyncio/test_base_events.py +++ b/Lib/test/test_asyncio/test_base_events.py @@ -476,7 +476,7 @@ def test_run_until_complete_loop(self): 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 @@ def throw(): 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.