Skip to content

Commit f343f1c

Browse files
author
Arran Cudbard-Bell
committed
Fix SystemError in _DeferredCall.get() under GC pressure
CPython builds the argument tuple incrementally via _PyTuple_Resize when unpacking a generator: f(*(gen)). _PyTuple_Resize guards that Py_REFCNT(v) == 1 before resizing a non-empty tuple. Under sustained allocation pressure a GC cycle can run between generator yields and temporarily increment the refcount on the partial tuple, causing _PyTuple_Resize to call PyErr_BadInternalCall(). Change the generator expression to a list comprehension. CPython builds the list first and passes it to CALL_FUNCTION_EX, which takes the PySequence_Fast list path and never calls _PyTuple_Resize.
1 parent b0cc432 commit f343f1c

2 files changed

Lines changed: 57 additions & 1 deletion

File tree

sdks/python/apache_beam/runners/worker/sdk_worker.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1454,7 +1454,7 @@ def wait(self, timeout=None):
14541454

14551455
def get(self, timeout=None):
14561456
# type: (Optional[float]) -> T
1457-
return self._func(*(arg.get(timeout) for arg in self._args))
1457+
return self._func(*[arg.get(timeout) for arg in self._args])
14581458

14591459
def set(self, value):
14601460
# type: (T) -> _Future[T]

sdks/python/apache_beam/runners/worker/sdk_worker_test.py

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -704,6 +704,62 @@ def testShortIdAssignment(self):
704704
% case.info)
705705

706706

707+
class DeferredCallTest(unittest.TestCase):
708+
"""Tests for _DeferredCall.get().
709+
710+
Background: the original implementation used a generator expression in the
711+
argument unpack position:
712+
713+
return self._func(*(arg.get(timeout) for arg in self._args))
714+
715+
CPython builds the argument tuple incrementally via _PyTuple_Resize as it
716+
drains the generator. _PyTuple_Resize guards that Py_REFCNT(v) == 1 before
717+
resizing a non-empty tuple (Objects/tupleobject.c). Under sustained load,
718+
a GC cycle can run between generator yields and temporarily increment the
719+
refcount on the partially-built tuple, causing _PyTuple_Resize to call
720+
PyErr_BadInternalCall() and raise SystemError. This was observed in
721+
production workers (Python 3.11, Beam 2.73.0) at
722+
Objects/tupleobject.c:927.
723+
724+
The partial tuple is a C-level local inside PySequence_Tuple, so the race
725+
cannot be triggered deterministically from pure Python. The tests here
726+
verify correct behaviour; the crash itself requires CPython internal timing
727+
or a debug build to reproduce reliably.
728+
729+
Fix: change the generator to a list comprehension. CPython builds the list
730+
first and passes it to CALL_FUNCTION_EX, which calls PySequence_Fast on a
731+
list (a no-op path that does not call _PyTuple_Resize).
732+
"""
733+
def test_get_single_arg(self):
734+
f = sdk_worker._Future().set(42)
735+
call = sdk_worker._DeferredCall(lambda x: x, f)
736+
self.assertEqual(call.get(), 42)
737+
738+
def test_get_multiple_args(self):
739+
futures = [sdk_worker._Future().set(i) for i in range(5)]
740+
call = sdk_worker._DeferredCall(lambda *args: sum(args), *futures)
741+
self.assertEqual(call.get(), sum(range(5)))
742+
743+
def test_get_non_future_args_are_wrapped(self):
744+
# __init__ wraps non-Future values in _Future().set(v); get() must work.
745+
call = sdk_worker._DeferredCall(lambda x, y: x * y, 3, 7)
746+
self.assertEqual(call.get(), 21)
747+
748+
def test_get_mixed_future_and_value_args(self):
749+
a = sdk_worker._Future().set(10)
750+
call = sdk_worker._DeferredCall(lambda x, y: x + y, a, 5)
751+
self.assertEqual(call.get(), 15)
752+
753+
def test_get_zero_args(self):
754+
call = sdk_worker._DeferredCall(lambda: 99)
755+
self.assertEqual(call.get(), 99)
756+
757+
def test_get_preserves_return_value_type(self):
758+
f = sdk_worker._Future().set({'key': 'val'})
759+
call = sdk_worker._DeferredCall(lambda d: d, f)
760+
self.assertEqual(call.get(), {'key': 'val'})
761+
762+
707763
def monitoringInfoMetadata(info):
708764
return {
709765
descriptor.name: value

0 commit comments

Comments
 (0)