[New-bugs-announce] [issue33110] Adding a done callback to a concurrent.futures Future once it has already completed, may raise an exception, contrary to docs

[Sam Martin]

New submission from Sam Martin <nivekkas at gmail.com>:

Whilst working with concurrent.futures and ThreadPoolExecutors, my colleague and I have noted some undocumented behaviour.

When adding a done_callback to a future that has already completed, we note that that callback is executed directly, outside of any try...except statement.  However, both the docs and the _invoke_callbacks methods will wrap any done_callbacks in a try...except statement which logs the result and returns.

Could we either update the documentation to mention that callback behaviour may raise if added to an already completed future, or simply add the same try...except wrapping that the _invoke_callback method already uses please? (Preferably the latter)

The two pieces of the futures library I am referring to can be viewed here:
_invoke_callbacks: https://github.com/python/cpython/blob/master/Lib/concurrent/futures/_base.py#L323

add_done_callback: https://github.com/python/cpython/blob/master/Lib/concurrent/futures/_base.py#L403

I would note, that the test code which covers this area of the code, doesn't currently exercise this particular condition.  The closest test I could find is test_done_callback_already_failed, which checks that a callback can retrieve an exception from a future, but it does not validate what happens when a callback raises when the future it is attached to is already complete.
Source: https://github.com/python/cpython/blob/c3d9508ff22ece9a96892b628dd5813e2fb0cd80/Lib/test/test_concurrent_futures.py#L1012

The other test closely related is test_done_callback_raises, however this doesn't check the behaviour of a callback when added to an already completed future.  We should be able to simulate this by moving the f.set_result line to above the f.add_done_callback lines?

Source: https://github.com/python/cpython/blob/c3d9508ff22ece9a96892b628dd5813e2fb0cd80/Lib/test/test_concurrent_futures.py#L990

----------
components: Library (Lib)
messages: 314151
nosy: samm
priority: normal
severity: normal
status: open
title: Adding a done callback to a concurrent.futures Future once it has already completed, may raise an exception, contrary to docs
type: behavior
versions: Python 3.6

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue33110>
_______________________________________