Skip to content

Commit 72a88d7

Browse files
serhiy-storchakasindriggraingert
authored
gh-83371: Fix deadlock when a Pool callback raises an exception (GH-155777)
The exception killed the thread which handles results, so that the pool hung forever. It is now the result of the job and is raised by AsyncResult.get(), with the original error as its context. Co-authored-by: Sindri Guðmundsson <sindrigudmundsson@gmail.com> Co-authored-by: Thomas Grainger <tagrain@gmail.com>
1 parent 9111820 commit 72a88d7

3 files changed

Lines changed: 157 additions & 17 deletions

File tree

Lib/multiprocessing/pool.py

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -763,6 +763,16 @@ def __enter__(self):
763763
def __exit__(self, exc_type, exc_val, exc_tb):
764764
self.terminate()
765765

766+
def _chain_context(exc, context):
767+
'Set context as the context of exc, avoiding a cycle.'
768+
seen = {id(context)}
769+
while exc is not None and id(exc) not in seen:
770+
seen.add(id(exc))
771+
if exc.__context__ is None:
772+
exc.__context__ = context
773+
return
774+
exc = exc.__context__
775+
766776
#
767777
# Class whose instances are returned by `Pool.apply_async()`
768778
#
@@ -800,13 +810,25 @@ def get(self, timeout=None):
800810

801811
def _set(self, i, obj):
802812
self._success, self._value = obj
803-
if self._callback and self._success:
804-
self._callback(self._value)
805-
if self._error_callback and not self._success:
806-
self._error_callback(self._value)
807-
self._event.set()
808-
del self._cache[self._job]
809-
self._pool = None
813+
try:
814+
if self._success:
815+
if self._callback:
816+
self._callback(self._value)
817+
else:
818+
if self._error_callback:
819+
self._error_callback(self._value)
820+
except BaseException as exc:
821+
# A failed callback becomes the result of the job. If it
822+
# propagated, it would kill the result handler thread.
823+
if not self._success:
824+
# do not lose the original error
825+
_chain_context(exc, self._value)
826+
self._success = False
827+
self._value = exc
828+
finally:
829+
self._event.set()
830+
del self._cache[self._job]
831+
self._pool = None
810832

811833
__class_getitem__ = classmethod(types.GenericAlias)
812834

@@ -837,23 +859,33 @@ def _set(self, i, success_result):
837859
if success and self._success:
838860
self._value[i*self._chunksize:(i+1)*self._chunksize] = result
839861
if self._number_left == 0:
840-
if self._callback:
841-
self._callback(self._value)
842-
del self._cache[self._job]
843-
self._event.set()
844-
self._pool = None
862+
try:
863+
if self._callback:
864+
self._callback(self._value)
865+
except BaseException as exc:
866+
self._success = False
867+
self._value = exc
868+
finally:
869+
del self._cache[self._job]
870+
self._event.set()
871+
self._pool = None
845872
else:
846873
if not success and self._success:
847874
# only store first exception
848875
self._success = False
849876
self._value = result
850877
if self._number_left == 0:
851878
# only consider the result ready once all jobs are done
852-
if self._error_callback:
853-
self._error_callback(self._value)
854-
del self._cache[self._job]
855-
self._event.set()
856-
self._pool = None
879+
try:
880+
if self._error_callback:
881+
self._error_callback(self._value)
882+
except BaseException as exc:
883+
_chain_context(exc, self._value)
884+
self._value = exc
885+
finally:
886+
del self._cache[self._job]
887+
self._event.set()
888+
self._pool = None
857889

858890
#
859891
# Class whose instances are returned by `Pool.imap()`

Lib/test/_test_multiprocessing.py

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3474,12 +3474,114 @@ def test_resource_warning(self):
34743474
pool = None
34753475
support.gc_collect()
34763476

3477+
class CallbackError(Exception): pass
3478+
3479+
class CallbackBaseException(BaseException): pass
3480+
34773481
def raising():
34783482
raise KeyError("key")
34793483

3484+
def raising_map(x):
3485+
raise KeyError("key")
3486+
3487+
def reraise(exc):
3488+
raise exc
3489+
3490+
def raise_with_context(exc):
3491+
try:
3492+
raise ZeroDivisionError
3493+
except ZeroDivisionError:
3494+
raise CallbackError('callback failed')
3495+
34803496
def unpickleable_result():
34813497
return lambda: 42
34823498

3499+
class _TestPoolCallbackErrors(BaseTestCase):
3500+
ALLOWED_TYPES = ('processes', )
3501+
3502+
@staticmethod
3503+
def _raise(value):
3504+
raise CallbackError('callback failed')
3505+
3506+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3507+
def test_apply_async_callback_raises(self):
3508+
with multiprocessing.Pool(1) as p:
3509+
res = p.apply_async(sqr, (7,), callback=self._raise)
3510+
with self.assertRaises(CallbackError):
3511+
res.get(support.SHORT_TIMEOUT)
3512+
# the pool is still usable
3513+
self.assertEqual(p.apply(sqr, (3,)), 9)
3514+
self.assertTrue(p._result_handler.is_alive())
3515+
3516+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3517+
def test_apply_async_callback_raises_base_exception(self):
3518+
def raise_base(value):
3519+
raise CallbackBaseException
3520+
with multiprocessing.Pool(1) as p:
3521+
res = p.apply_async(sqr, (7,), callback=raise_base)
3522+
with self.assertRaises(CallbackBaseException):
3523+
res.get(support.SHORT_TIMEOUT)
3524+
# the pool did not hang
3525+
self.assertEqual(p.apply(sqr, (3,)), 9)
3526+
3527+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3528+
def test_apply_async_error_callback_raises(self):
3529+
with multiprocessing.Pool(1) as p:
3530+
res = p.apply_async(raising, error_callback=self._raise)
3531+
with self.assertRaises(CallbackError) as cm:
3532+
res.get(support.SHORT_TIMEOUT)
3533+
# the original error is not lost
3534+
self.assertIsInstance(cm.exception.__context__, KeyError)
3535+
self.assertEqual(p.apply(sqr, (3,)), 9)
3536+
3537+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3538+
def test_apply_async_error_callback_reraises(self):
3539+
with multiprocessing.Pool(1) as p:
3540+
res = p.apply_async(raising, error_callback=reraise)
3541+
with self.assertRaises(KeyError) as cm:
3542+
res.get(support.SHORT_TIMEOUT)
3543+
# the error is not its own context
3544+
self.assertIsNone(cm.exception.__context__)
3545+
self.assertEqual(p.apply(sqr, (3,)), 9)
3546+
3547+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3548+
def test_map_async_error_callback_reraises(self):
3549+
with multiprocessing.Pool(1) as p:
3550+
res = p.map_async(raising_map, [0], error_callback=reraise)
3551+
with self.assertRaises(KeyError) as cm:
3552+
res.get(support.SHORT_TIMEOUT)
3553+
self.assertIsNone(cm.exception.__context__)
3554+
self.assertEqual(p.apply(sqr, (3,)), 9)
3555+
3556+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3557+
def test_apply_async_error_callback_raises_with_context(self):
3558+
# the original error is kept at the end of the context chain
3559+
with multiprocessing.Pool(1) as p:
3560+
res = p.apply_async(raising, error_callback=raise_with_context)
3561+
with self.assertRaises(CallbackError) as cm:
3562+
res.get(support.SHORT_TIMEOUT)
3563+
context = cm.exception.__context__
3564+
self.assertIsInstance(context, ZeroDivisionError)
3565+
self.assertIsInstance(context.__context__, KeyError)
3566+
self.assertEqual(p.apply(sqr, (3,)), 9)
3567+
3568+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3569+
def test_map_async_callback_raises(self):
3570+
with multiprocessing.Pool(1) as p:
3571+
res = p.map_async(sqr, list(range(3)), callback=self._raise)
3572+
with self.assertRaises(CallbackError):
3573+
res.get(support.SHORT_TIMEOUT)
3574+
self.assertEqual(p.apply(sqr, (3,)), 9)
3575+
3576+
@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
3577+
def test_map_async_error_callback_raises(self):
3578+
with multiprocessing.Pool(1) as p:
3579+
res = p.map_async(raising_map, [0], error_callback=self._raise)
3580+
with self.assertRaises(CallbackError) as cm:
3581+
res.get(support.SHORT_TIMEOUT)
3582+
self.assertIsInstance(cm.exception.__context__, KeyError)
3583+
self.assertEqual(p.apply(sqr, (3,)), 9)
3584+
34833585
class _TestPoolWorkerErrors(BaseTestCase):
34843586
ALLOWED_TYPES = ('processes', )
34853587

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Fix a deadlock in :class:`multiprocessing.pool.Pool` when *callback* or
2+
*error_callback* raises an exception.
3+
It killed the thread which handles results, so that the pool hung forever.
4+
The exception is now the result of the job,
5+
as an error raised while iterating the input,
6+
and is raised by :meth:`!AsyncResult.get`.

0 commit comments

Comments
 (0)