classification
Title: run_coroutine_threadsafe uses wrong TimeoutError
Type: Stage:
Components: Documentation Versions: Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: docs@python Nosy List: aeros, asvetlov, docs@python, janust, nickgaya, pfctdayelise, yselivanov
Priority: normal Keywords:

Created on 2019-12-11 22:04 by janust, last changed 2020-09-28 21:15 by nickgaya.

Messages (5)
msg358281 - (view) Author: (janust) Date: 2019-12-11 22:04
https://docs.python.org/3.8/library/asyncio-task.html#asyncio.run_coroutine_threadsafe has a code example that catches a asyncio.TimeoutError from run_coroutine_threadsafe. In Python 3.7, this exception was equal to concurrent.futures.TimeoutError, but since https://github.com/python/cpython/commit/431b540bf79f0982559b1b0e420b1b085f667bb7 that is not the case anymore.
msg358296 - (view) Author: Kyle Stanley (aeros) * (Python committer) Date: 2019-12-12 09:41
Thanks for letting us know, janust. I confirmed that `asyncio.TimeoutError` no longer works for the code example in 3.8 and that replacing it with `concurrent.futures.TimeoutError` works correctly.

Before moving forward with a PR to the docs, I think we should wait for feedback from Yury to check if this behavior is intentional for `run_coroutine_threadsafe()`, I'll add him to the nosy list.

To me, this seems more like a side effect of `asyncio.TimeoutError` being changed to BaseException rather than an intended consequence. The main purpose of that change was to ensure that TimeoutError wasn't unintentionally suppressed by generic try-except blocks. IMO, it seems rather counter-intuitive to have to specify `concurrent.futures.TimeoutError` when using a timeout for the future returned by `run_coroutine_threadsafe()`. I'm not certain that it can be avoided though, other than by changing it to return an instance of asyncio.Future instead of concurrent.futures.Future.
msg358297 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-12-12 10:05
futures._chain_future() should convert exceptions.
Seems _convert_future_exc() does this work already but maybe it fails somehow.
We need to investigate more.
msg377595 - (view) Author: pfctdayelise (pfctdayelise) * Date: 2020-09-28 11:16
Can we at least update the docs in the meantime? It's not great having incorrect docs. Then if a fix is made to revert the behaviour, it can include updating the docs back.
msg377629 - (view) Author: Nick Gaya (nickgaya) Date: 2020-09-28 21:15
@msg358296:

> IMO, it seems rather counter-intuitive to have to specify `concurrent.futures.TimeoutError` when using a timeout for the future returned by `run_coroutine_threadsafe()`.

I think that's expected, given that the function returns a `concurrent.futures.Future` object. The `Future.result()` method behaves the same regardless of the executor.

@avsetlov:

> futures._chain_future() should convert exceptions. Seems _convert_future_exc() does this work already but maybe it fails somehow. We need to investigate more.

The `_convert_future_exc()` only converts `concurrent.futures` exceptions to `asyncio` exceptions, not the reverse. I'm not sure if this is intentional or not.

With the current behavior there are two types of timeout errors that can occur in the example snippet:

* If the coroutine itself throws an `asyncio.exceptions.TimeoutError`, this will be propagated as-is to the `concurrent.futures.Future` object and thrown by `future.result()`.

* If the coroutine does not terminate within the timeout supplied to `future.result()`, then the method will throw a `concurrent.futures.TimeoutError` without changing the state of the future or the associated coroutine.

It's only necessary to cancel the future in the second case, as in the first case it's already in a finished state. So the example should catch `concurrent.futures.TimeoutError` rather than `asyncio.TimeoutError`.
History
Date User Action Args
2020-09-28 21:15:00nickgayasetnosy: + nickgaya
messages: + msg377629
2020-09-28 11:16:06pfctdayelisesetnosy: + pfctdayelise
messages: + msg377595
2019-12-12 10:05:03asvetlovsetnosy: + asvetlov
messages: + msg358297
2019-12-12 09:41:20aerossetnosy: + yselivanov, aeros
messages: + msg358296
2019-12-11 22:04:07janustcreate