Adding a done callback to a concurrent.futures Future once it has already completed, may raise an exception, contrary to docs

[ABitMoreDepth]

BPO	33110
Nosy	@brianquinlan, @pitrou, @miss-islington, @ABitMoreDepth, @tirkarthi
PRs	bpo-33110: Catches errors raised when running add_done_callback on already completed futures. #13141 [3.7] bpo-33110: Catch errors raised when running add_done_callback on already completed futures (GH-13141) #13508 [3.6] bpo-33110: Catch errors raised when running add_done_callback on already completed futures (GH-13141) #13509

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-05-23.07:40:04.581>
created_at = <Date 2018-03-20.16:12:43.307>
labels = ['3.7', '3.8', 'type-bug', 'library']
title = 'Adding a done callback to a concurrent.futures Future once it has already completed, may raise an exception, contrary to docs'
updated_at = <Date 2019-05-23.07:40:04.581>
user = 'https://github.com/ABitMoreDepth'

bugs.python.org fields:

activity = <Date 2019-05-23.07:40:04.581>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2019-05-23.07:40:04.581>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2018-03-20.16:12:43.307>
creator = 'samm'
dependencies = []
files = []
hgrepos = []
issue_num = 33110
keywords = ['patch']
message_count = 4.0
messages = ['314151', '343242', '343249', '343279']
nosy_count = 5.0
nosy_names = ['bquinlan', 'pitrou', 'miss-islington', 'samm', 'xtreak']
pr_nums = ['13141', '13508', '13509']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33110'
versions = ['Python 3.7', 'Python 3.8']

[ABitMoreDepth]

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:

cpython/Lib/test/test_concurrent_futures.py

Line 1012 in c3d9508

def test_done_callback_already_successful(self):

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:

cpython/Lib/test/test_concurrent_futures.py

Line 990 in c3d9508

def test_done_callback_raises(self):

[pitrou]

New changeset 2a3a2ec by Antoine Pitrou (Sam Martin) in branch 'master':
bpo-33110: Catch errors raised when running add_done_callback on already completed futures (GH-13141)
2a3a2ec

[miss-islington]

New changeset b73c21c by Miss Islington (bot) in branch '3.7':
bpo-33110: Catch errors raised when running add_done_callback on already completed futures (GH-13141)
b73c21c

[pitrou]

Thank you Sam for your contribution!