_result_handler dies on raised exceptions [multiprocessing]

@sindrig

BPO	39190
Nosy	@sindrig
PRs	gh-83371: Fix deadlock when callback raises #17795
Files	test_pool_error.py: Steps to reproduce

^{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 = None
created_at = <Date 2020-01-02.12:26:53.919>
labels = ['3.10', 'library', '3.9', 'type-crash', '3.11']
title = '_result_handler dies on raised exceptions [multiprocessing]'
updated_at = <Date 2021-09-19.22:01:28.549>
user = 'https://github.com/sindrig'

bugs.python.org fields:

activity = <Date 2021-09-19.22:01:28.549>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2020-01-02.12:26:53.919>
creator = 'sindrig'
dependencies = []
files = ['48818']
hgrepos = []
issue_num = 39190
keywords = ['patch']
message_count = 1.0
messages = ['359194']
nosy_count = 1.0
nosy_names = ['sindrig']
pr_nums = ['17795']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue39190'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

Raising an Exception in a callback handler of apply_async and/or map_async will kill the _result_handler thread. This causes unexpected behavior as all subsequent callbacks won't be called and worse, pool.join will deadlock.

The documentation states that callbacks should return immediately but it does not warn the user against raising an exception.

Attached are steps to reproduce.

it doesn't change the computed result, only the exception.

because this bug totally hoses the multiprocessing Pool were sort of free to change the behaviour however we want. It doesn't matter if code could break it's already broken! So we could raise an ExceptionGroup out of AsyncResult.get()

or perhaps we could mark the Pool as broken so all methods and AsyncResult.get() raise BrokenPoolException(pool_id), then in the __exit__ of the Pool we can detect the BrokenPoolException and raise the exception from the callback.

it doesn't change the computed result, only the exception.

Sure, but from the point of view of the calling code it is the same thing. They call get and get an exception instead of the correct value: a different result. However, I do take your point: the calling code should know something is broken, so we should raise. Calling the default hook will probably mean the error is logged at best, which is too easy to overlook.

because this bug totally hoses the multiprocessing Pool were sort of free to change the behaviour however we want. It doesn't matter if code could break it's already broken! So we could raise an ExceptionGroup out of AsyncResult.get()

Right. I've never used exception groups but AIUI starting to throw them from a method that previously didn't is a breaking API change. However, it doesn't really matter in this case, since if it happens the program would have been badly broken previously anyway. Is that correct?

So, the semantics of get would be:

No change if no callback raises (i.e. it will return the call's value or raise a naked exception as appropriate)
If callback raises, get will raise an exception group containing the callback exception
If error_callback raises, get will raise an exception group containing the callback exception and the original exception

Existing exception handling code will not catch these new exception groups, which is good: we wouldn't want them to be treated as "normal" failures.

or perhaps we could mark the Pool as broken so all methods and AsyncResult.get() raise BrokenPoolException(pool_id), then in the __exit__ of the Pool we can detect the BrokenPoolException and raise the exception from the callback.

I like the sound of this, but it does get a bit complicated.

If we are __exit__ing because an exception was raised and a callback has failed, we presumably want to include the exception that was raised in the exception group, otherwise it would be lost. However, it could be the exception group itself being raised from get, in which case we probably don't want it in the group twice. Right?

E.g. consider exiting with a "normal" error (note if we didn't do the get the context would probably exit before the task can run, meaning the naked ValueError would propagate out):

with Pool() as pool:
    res = pool.async_apply(foo, callback=raising)

    # Ensure the task has run and set the error, but don't let it propagate out
    try:
        res.get()
    except CallbackError:
        pass

    raise ValueError("error")

...and exiting via callback error:

with Pool() as pool:
    pool.apply_async(foo, callback=raising).get()

We can't avoid the duplication by checking if it is already in the list of sub-exceptions: that fails for managers which use proxies and unpickles the same exception group into two separate instances. Instead I've skipped adding the exception if it is a CallbackError. In theory a different one could have been raised and hence will be lost, e.g. if the code does a get on an async result from a different pool, but that seems pathological enough that we can probably ignore it.

I ~~have~~ thought I had a working implementation of this, with unit tests for all the variations of the new behaviour, but it is significantly more complex than the original fix. I'll push it out so you can have a look and see what you think. If we go with this approach I'll also need to update the API documentation and the NEWS entry as well, but I'll wait until I hear your thoughts before proceeding with that.

Edited to add: looks like it is not working properly after all, I will continue to debug tomorrow, but let me know if you have any comments on the approach in general in the meantime.

(the interpreter doesn't crash per se, just that the interpreter hangs; but no segmentation fault occurs, which is why I changed the labels)

@graingert

The CI tests are failing on free-threading builds due to the Barrier object not being cleaned up properly. Part of the problem is presumably that storing the exceptions thrown from callbacks creates a reference cycle. Their stack traces necessarily hold references to locals, which contain references to the objects containing them.

However, the test code does an explicit gc before checking for shared resource leaks, and AFAICT all the managers/processes/threads should have been terminated and joined, so I'm not sure why the cycle is not being collected and the Barrier objects finalized nonetheless.

I cannot reproduce the failure here (in a free-threading build, using the same configure options and test command-line as the CI system), which is unexpected and makes debugging difficult. I could enable logging/add diagnostics and push out exploratory commits to CI to see what happens, but I'm reluctant to spam the system like that.

At this point, my preference is to revert back to just dealing with callback exceptions via the exception hook, however I'm open to other options.

What do you think @graingert and/or anyone else who cares to weigh in?

Merge branch 'main' of https://github.com/python/cpython into pythong…

There are four PRs for this now.
GH-17795 and GH-131813 report the exception (on stderr and via threading.excepthook()) and leave the job successful; GH-131883 and GH-155777 make the failed callback the result of the job, raised by AsyncResult.get().

I wrote GH-155777 before reading this issue and did not know about GH-131813 and GH-131883, sorry for the duplication. It follows the design of GH-131883, with two differences: if the error callback raises, the original error is kept at the end of the context chain, and there are tests for the callback and the error callback of apply_async() and map_async(), for a BaseException, for an error callback which re-raises the exception it gets, and for one which raises with its own context. GH-131883 replaces the original error with the callback error, so the failure which the user actually waited for is lost.

As for losing the computed result: it can be lost anyway, if an error occurs after computing and before sending, during serialization or deserialization, or in the user code after getting it. Pool already turns an unpicklable result into MaybeEncodingError, and get() raises it instead of returning the value which was computed successfully. A failing callback is not different from any other error which occurs after the computation.

About the reference cycles: storing the exception does not create them. The cycle is created when get() raises the exception in the caller frame, and that already happens for a failed job. The tests leave no uncollectable garbage and no leaked resources.

gh-83371: Fix deadlock when a Pool callback raises an exception (GH-1…

Not backported, this is too large change. This change just improves QoL. Normally, callbacks do not raise. Users should ensure that the callbacks do not fail if they want to avoid a deadlock in old versions. They always can do this by catching exceptions in the callbacks.

pythongh-83371: Fix deadlock when a Pool callback raises an exception (…

Navigation Menu

Repository navigation

_result_handler dies on raised exceptions [multiprocessing] #83371

Linked PRs

8 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

_result_handler dies on raised exceptions [multiprocessing] #83371

Description

Linked PRs

Activity

sindrig commented on Jan 2, 2020

8 remaining items

graingert commented on Mar 31, 2025

duaneg commented on Apr 1, 2025

picnixz commented on Apr 1, 2025

duaneg commented on Apr 7, 2025

serhiy-storchaka commented on Aug 14, 2026

serhiy-storchaka commented on Aug 29, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions