Release the GIL during the activity heartbeat core call#1643
Open
odedkedemdreeze wants to merge 1 commit into
Open
Release the GIL during the activity heartbeat core call#1643odedkedemdreeze wants to merge 1 commit into
odedkedemdreeze wants to merge 1 commit into
Conversation
record_activity_heartbeat was a synchronous pyo3 fn that held the GIL while core's record_heartbeat blocked on the outstanding-activity lock. Task starts invoke a custom slot supplier's mark_slot_used (which acquires the GIL) while holding that same lock, so any worker with a custom slot supplier and heartbeating activities could deadlock permanently (ABBA on GIL vs. lock). Decode the heartbeat proto with the GIL held (it borrows the Python bytes), then detach from the GIL for the core call so the cycle cannot form. Fixes temporalio#1642 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
“Oded seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
1 similar comment
|
“Oded seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
WorkerRef::record_activity_heartbeatin the pyo3 bridge now detaches from the GIL (Python::detach, formerlyallow_threads) for the call into core. The heartbeat proto is still decoded with the GIL held (it borrows the Pythonbytes), so only owned data crosses the detach boundary.Also adds a regression test: a trivial custom slot supplier + a workflow fanning out heartbeating activities (
tests/worker/test_worker.py::test_custom_slot_supplier_with_heartbeating_activities).Why?
Fixes the permanent worker deadlock described in #1642: any worker using a
CustomSlotSupplierwhose activities heartbeat can wedge forever.record_heartbeatblocked on theoutstanding_activity_tasksmutex.mark_slot_used— whose bridge wrapper unconditionally acquires the GIL — while core holds that same mutex (guard alive duringinsert(...)argument evaluation).Classic ABBA: thread A holds the mutex and wants the GIL; thread B holds the GIL and wants the mutex. Releasing the GIL during the core call means the cycle cannot form, and as a bonus a heartbeat contending on that mutex no longer freezes the entire Python process (event loop and all unrelated threads) for the duration.
The other half of the cycle (core invoking lang callbacks while holding internal locks, including via Rust temporary-lifetime guard extension in
insert(...)argument position) is described in #1642 for the maintainers' consideration; this PR fixes the Python side, which alone breaks the cycle.Checklist
Closes [Bug] Permanent worker deadlock: any CustomSlotSupplier + heartbeating activities (GIL ↔ sdk-core mutex ABBA) #1642
How was this tested:
New regression test
test_custom_slot_supplier_with_heartbeating_activities(600 heartbeating activities through a capped custom slot supplier). Passes in ~13s with this change. Note: on regression it hangs rather than fails — the deadlock freezes the event loop, so no in-process timeout can fire (the deterministic non-hanging variant of this test would belong in sdk-core, where the lock ordering can be unit-tested directly).The reproducer from [Bug] Permanent worker deadlock: any CustomSlotSupplier + heartbeating activities (GIL ↔ sdk-core mutex ABBA) #1642 (8000 heartbeating activities, no-op custom slot supplier), unpatched
temporalio==1.30.0:Same reproducer against this branch (editable install, patched bridge):
cargo fmt --check,cargo clippy -- -D warnings(bridge),ruff check --select I/ruff format --checkon the touched test file: clean.All four
*slot_supplier*tests intests/worker/test_worker.pypass locally.None — behavior-preserving fix, no API change.