Fix deadlock in rayon pool - #422
Draft
seanyoung wants to merge 1 commit into
Draft
Conversation
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.
Description
Block-STM workers ran on a rayon thread pool (
RAYON_EXEC_POOL), which makes adeadlock reachable from any user transaction that calls a rayon-using Move native.
A rayon worker that blocks is not idle. Rayon's
wait_untilloop keeps it busy bystealing other jobs from its own pool onto that thread. So a worker that blocks
inside a Move native's nested
par_iter()—ark_ecMSM, pairing, multi-pairing,ark_ff::batch_inversionunderhash_to_structure— can pick up a sibling workertask, and that stolen task can then block on state the original task still holds:
the writer-preferring
RwLockover per-txn status inscheduler.rs, or a v1dependency condvar for the very transaction this thread had been executing. Neither
side progresses, and the scheduler still believes the transaction is executing.
Every precondition is present in this tree today:
executor_thread_pool.scope(...), two sites inexecutor.rs)parking_lot::RwLockontxn_statusandDependencyCondvarinscheduler.rsark-std 0.5.0carriesrayoninCargo.lock, so the natives reach itCRYPTOGRAPHY_ALGEBRA_NATIVES,BLS12_381_STRUCTURESandBN254_STRUCTURESareall default-enabled, so the path is reachable from an ordinary transaction
This makes Block-STM workers plain OS threads via
std::thread::scope. Threads notregistered with rayon cannot have rayon jobs stolen onto them; a native's nested
par_iter()runs on rayon's global pool while the worker parks on OS primitives.That closes the whole class structurally — for every native, including ones nobody
remembered to isolate — rather than requiring each rayon-using native to opt in.
With the pool no longer driving workers,
RAYON_EXEC_POOLhad no remaining purposeand is removed, along with the plumbing that existed only to carry it:
RAYON_EXEC_POOLnum_cpus-threadpar_exec-*poolBlockExecutor::new(config, pool, hook)new(config, hook)execute_block_on_thread_pool(pool, …)execute_block(…); the wrapper only injected the poolcreate_executor_thread_pool()+ 12 per-test poolsexecute_block_parallel/run_tests_with_groupsTwo call sites needed a real decision rather than a mechanical edit:
FakeExecutorheld a pool but only ever readcurrent_num_threads()off it.Replaced with
concurrency_level: usize, which is what the surrounding commentalready said it wanted.
from_genesis_with_existing_thread_poolbecomesfrom_genesis_with_concurrency_level; the four fuzz targets that used it eachbuilt their pool with
num_threads(FUZZER_CONCURRENCY_LEVEL), so they now passthat constant directly — no behavior change.
NATIVE_EXECUTOR_POOL(executor-benchmark) is kept. UnlikeRAYON_EXEC_POOLit has independent
.install()users inaptos_vm_uncoordinated.rsandparallel_uncoordinated_block_executor.rs. Only the argument was dropped.The sharded executor's pools are likewise kept — they drive the cross-shard commit
receiver and aggregator, not Block-STM workers.
Relationship to upstream
Motivated by aptos-labs/aptos-core@ad7011f771, which fixes the same deadlock class.
This implementation is independent, written from the failure mode rather than ported,
and diverges deliberately:
(~423 lines, using
mem::transmuteto forge a'staticlifetime on the workclosure, plus
catch_unwind, aBarrier, and a panic slot). This usesstd::thread::scope, which gives lifetime safety and panic propagation from thecompiler instead.
their tree because their
with_native_rayonkeeps its per-caller rayon pool in athread_local!, which scoped workers would rebuild every block. We do not carrywith_native_rayon, so natives use rayon's process-wide global pool and nothing isrebuilt. Measured cost of the simpler approach here: ~185us per parallel block for
16 workers, scaling down with block size (
num_workersisconcurrency_level.min(num_txns / 2).max(2), so a small block spawns 2 threads).If we later adopt
with_native_rayonfor defence in depth, that trade-off flips anda persistent pool becomes worth revisiting.
How Has This Been Tested?
cargo check --workspace --all-targets— clean. This is the load-bearing check:per-crate checks miss
fuzzer-fuzz, which is a separate package fromfuzzer.aptos-block-executorlib suite, 237/237: 98 combinatorial proptests (therandomised parallel-execution stress: contention, aborts, gas limits, delayed
fields, resource groups), 113 scheduler/module tests, 26 unit + invariant tests.
executor::worker_thread_tests::workers_are_not_rayon_registeredpins the actual invariant: every worker sees
rayon::current_thread_index() == None,driven from inside a rayon pool — the worst case, and the configuration the sharded
executor uses. It also asserts the driving thread is rayon-registered, so it
cannot pass vacuously.
Note for reviewers:
unit_tests::test_resource_group_deletionis flaky and it isnot caused by this change.
resource_group_bcs_fallbackarms the process-globalfailpoint
fail-point-resource-group-serializationwhile libtest runs other tests inparallel. Measured 6 failures in 8 runs both on this branch and with all files reverted
to the base commit — identical exposure — and 26/26 passes with
--test-threads=1.Worth a
serial_testguard in a separate PR (serial_testis already a workspace dep).Key Areas to Review
spawn_block_stm_workerinexecutor.rs— the core of the change. The doccomment carries the deadlock rationale; that reasoning is the thing to check.
.expect()onspawn_scopedmeans that if worker kof N fails to spawn,
thread::scopejoins workers0..k-1first — they executethe whole block with fewer workers, then the panic propagates. Not a hang and not
incorrect output (Block-STM has no barrier and no worker-count termination
condition;
num_workersis used only forworker_idbounds-checking and ascheduling heuristic), but it is a new failure mode in kind: with the pre-spawned
pool, thread exhaustion could not fail block execution. Upstream avoids this by
spawning all threads before dispatching any task.
blockstm-{id}; the rayon pool usedpar_exec-{i}and upstream kept that name. Nothing in-repo referencespar_execany more, but external runbooks or thread-dump tooling might — this bug was
originally diagnosed from a production thread dump. Happy to switch back.
AptosBlockExecutorWrapper::execute_blockwidened frompub(crate)topub,since it absorbed the public
execute_block_on_thread_pool.execute_blockfrom a rayon worker. Harmless now(the VM runs on std threads beneath it) and it is not wired into
aptos-node, butit is the one place the old shape survives.
Type of Change
BlockExecutor::newloses itsArc<rayon::ThreadPool>parameter,execute_block_on_thread_poolis removed in favour ofexecute_block, andFakeExecutor::from_genesis_with_existing_thread_poolbecomesfrom_genesis_with_concurrency_level. All in-tree callers are updated.Which Components or Systems Does This Change Impact?
Checklist
Follow-ups (not in this PR)
aptos_rayon_execution_secondsstill names and describes a rayon pool that nolonger exists. Left as-is for dashboard continuity — upstream
maincarries thesame misnomer — but the description is now false and should be corrected.
serial_testguard for the failpoint-using tests described above.with_native_rayonfor defence in depth, so the isolation does notdepend solely on workers staying off rayon.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.