classification
Title: Adding a done callback to a concurrent.futures Future once it has already completed, may raise an exception, contrary to docs
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: bquinlan, miss-islington, pitrou, samm, xtreak
Priority: normal Keywords: patch

Created on 2018-03-20 16:12 by samm, last changed 2019-05-23 07:40 by pitrou. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13141 merged samm, 2019-05-06 22:25
PR 13508 merged miss-islington, 2019-05-22 21:29
PR 13509 closed miss-islington, 2019-05-22 21:29
Messages (4)
msg314151 - (view) Author: Sam Martin (samm) * Date: 2018-03-20 16:12
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
msg343242 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-05-22 21:29
New changeset 2a3a2ece502c05ea33c95dd0db497189e0354bfd by Antoine Pitrou (Sam Martin) in branch 'master':
bpo-33110: Catch errors raised when running add_done_callback on already completed futures (GH-13141)
https://github.com/python/cpython/commit/2a3a2ece502c05ea33c95dd0db497189e0354bfd
msg343249 - (view) Author: miss-islington (miss-islington) Date: 2019-05-22 22:02
New changeset b73c21c0be7b42de6a88d67408249c8ec46e28f7 by Miss Islington (bot) in branch '3.7':
bpo-33110: Catch errors raised when running add_done_callback on already completed futures (GH-13141)
https://github.com/python/cpython/commit/b73c21c0be7b42de6a88d67408249c8ec46e28f7
msg343279 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-05-23 07:40
Thank you Sam for your contribution!
History
Date User Action Args
2019-05-23 07:40:04pitrousetstatus: open -> closed

messages: + msg343279
stage: patch review -> resolved
2019-05-22 22:02:27miss-islingtonsetnosy: + miss-islington
messages: + msg343249
2019-05-22 21:32:17pitrousetresolution: fixed
versions: + Python 3.7, Python 3.8, - Python 3.6
2019-05-22 21:29:35miss-islingtonsetpull_requests: + pull_request13425
2019-05-22 21:29:23miss-islingtonsetpull_requests: + pull_request13424
2019-05-22 21:29:05pitrousetmessages: + msg343242
2019-05-06 22:25:48sammsetkeywords: + patch
stage: patch review
pull_requests: + pull_request13054
2018-09-20 12:39:53xtreaksetnosy: + xtreak
2018-03-22 00:00:44ned.deilysetnosy: + bquinlan, pitrou
2018-03-20 16:12:43sammcreate