re-land #8671: async_hooks lifecycle parity (defer crypto completions) - #8814
Merged
Conversation
…tions Re-lands #8671 with the pbkdf2 regression that caused its revert fixed. #8671 routed every native Node-style crypto completion callback through `timer::schedule_native_callback`, which enqueues the callback on the check phase inside its own async-hooks provider resource (`PBKDF2REQUEST`, `SCRYPTREQUEST`, ...) so `init`/`before`/`after`/`destroy` are observed around it. That is what Node does — `crypto.pbkdf2` completes on the libuv threadpool and never calls back synchronously — and it is what the async `randomBytes` form has done since #6430. `crypto::random::tests::native_dispatch_pbkdf2_value_form_fires_callback` still pinned the previous, non-Node behaviour: it asserted the callback had fired the instant `js_crypto_native_dispatch` returned. It failed deterministically (also in isolation — this was not suite-global state). Update the test to the delivery contract the sibling `randomBytes` test already encodes: the callback must NOT fire synchronously, the scheduled completion must be a pending ref'd event-loop handle (this is the same `js_callback_timer_has_pending` predicate the codegen event loop emits in `codegen/entry.rs`, so a real `util.promisify(crypto.pbkdf2)` caller cannot exit before it runs), and after one `js_callback_timer_tick` the callback must fire with a null error and a Buffer pointer.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (138)
📝 WalkthroughWalkthroughThis change expands async-hooks parity across runtime resources, promises, callbacks, networking, HTTP, events, filesystem APIs, workers, zlib, crypto, and WebCrypto. It also adds ChangesAsync-hooks runtime and resource lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant NativeAPI
participant ProviderRuntime
participant EventLoop
participant JavaScriptCallback
NativeAPI->>ProviderRuntime: initialize provider resource
NativeAPI->>EventLoop: schedule completion
EventLoop->>ProviderRuntime: enter resource scope
ProviderRuntime->>JavaScriptCallback: before and callback
JavaScriptCallback->>ProviderRuntime: after completion
ProviderRuntime->>ProviderRuntime: destroy provider resource
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 25, 2026
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
1 participant
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.
Re-lands #8671 (async_hooks lifecycle parity) with the one test that blocked it corrected.
I mischaracterised this PR when I reverted it — correcting that here
I backed #8671 out in #8798 and reported it as "breaks
crypto.pbkdf2's callback — a real caller would hang." That was wrong. #8671 madepbkdf2more Node-accurate, and a stale unit test caught the behaviour change.Node runs
crypto.pbkdf2on the libuv threadpool, so its(err, key)callback is never invoked synchronously. #8671 correctly moved native crypto completions from a synchronousjs_closure_call2toschedule_native_callback(..., "PBKDF2REQUEST"), so the callback executes inside its own async resource andasync_hooksobservesinit → before → after → destroyaround it — which is what the PR's own parity fixturetest-parity/node-suite/async_hooks/hooks/provider-pbkdf2-lifecycle.tsrequires, and what the asyncrandomBytesform has done since #6430.native_dispatch_pbkdf2_value_form_fires_callbackstill asserted the old synchronous delivery, i.e. it encoded non-Node behaviour.I also stated on this PR that the test "passes when filtered in isolation, and fails only in the full suite", and used that to argue for leaked suite-global state. That was an unverified inference — I had only checked isolation at commits that did not contain #8671, then carried the claim over. The failure is deterministic and order-independent. Apologies for sending anyone down that path.
The revert itself still stands as the right immediate action —
mainwas red and attribution was bisected — but the diagnosis attached to it was not.The fix
native_dispatch_pbkdf2_value_form_fires_callbacknow asserts the delivery contract its siblingrandomBytestest already encodes. It is strictly stronger than before — all three original assertions retained, two added:js_callback_timer_has_pending() != 0— the scheduled completion is a pending, ref'd event-loop handle. This is the direct answer to my "a caller would hang" worry: it is the same predicatecrates/perry-codegen/src/codegen/entry.rs:1158emits into the compiled program's event loop, so a realutil.promisify(crypto.pbkdf2)caller cannot exit before the callback runs;js_callback_timer_tick(), the callback fires with a null error and a Buffer pointer.Re-application and conflict resolution
A plain merge of the PR branch into current
mainis a no-op — the revert is the newer change onmain, so the 3-way merge discards the re-application.git revert-ing the revert squashf46932a78was also wrong, since that squash re-landed #8764. #8671's own diff was re-applied against974c57fca.One conflict, in
crates/perry-stdlib/src/fetch/mod.rs:main's #8805js_promise_new→js_promise_new_cross_threadvs #8671'srun_provider_completionrefactor. Resolved keeping #8671's structure withmain's cross-thread call — verified 15js_promise_new_cross_threadcall sites on bothmainand this branch, and zero remaining plainjs_promise_new(.Also audited: #8671 drops an
is_valid_obj_ptrguard inarray/subclass.rs::dense_layout_for_value, relying ontry_read_gc_header(...)?instead. That is safe de-duplication, not a lost check —try_read_gc_headercallsis_plausible_heap_addrfirst (rejecting null and implausible addresses) and additionally rejects small-buffer slab addresses, so it is strictly stronger than the guard removed.Validation (verified independently, not taken on report)
perry-stdlib --libfull suite:120 passed; 0 failed(was119 passed; 1 failed)1 passed; 0 failedperry-runtime --lib:2689 passed; 0 failedperry-codegen --lib:1249 passed; 0 failedCargo.tomlandCLAUDE.mduntouchedSummary by CodeRabbit
node:async_hookscompatibility, includingAsyncResource,AsyncLocalStorage, andEventEmitterAsyncResource.events.on()async iteration andevents.addAbortListener().once()support for HTTP request and incoming-message objects.data:URLs.