Conversation
How to use the Graphite Merge QueueAdd the label ready-to-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
Related to #2826 |
|
Can smbd look at the PR, please? |
There was a problem hiding this comment.
Pull Request Overview
Implements streaming support for object-type items in NAPI readable streams, fixing issues with object serialization in the stream processing pipeline.
- Fixed streaming functionality for object type items by updating the stream implementation
- Added example demonstrating object streaming with
FooandStreamItemstructs - Added comprehensive test coverage for object streaming functionality
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| examples/napi/src/stream.rs | Added object structs and streaming function implementation |
| examples/napi/index.d.cts | Generated TypeScript definitions for new streaming interfaces |
| examples/napi/index.cjs | Added export for new streaming function |
| examples/napi/example.wasi.cjs | Added WASI export for new streaming function |
| examples/napi/example.wasi-browser.js | Added browser export for new streaming function |
| examples/napi/tests/values.spec.ts | Added test cases for object streaming functionality |
| examples/napi/tests/snapshots/values.spec.ts.md | Updated test snapshots |
| crates/napi/src/bindgen_runtime/js_values/stream/read.rs | Fixed core streaming implementation for object types |
| examples/napi-cargo-test/CHANGELOG.md | Updated changelog formatting |
| crates/napi/CHANGELOG.md | Updated changelog formatting |
| crates/macro/CHANGELOG.md | Updated changelog formatting |
| crates/backend/CHANGELOG.md | Updated changelog formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
cursor review |
|
Any progress? |
|
Hello @Brooooooklyn ? |
|
@BadMachine can you make the CI all green first? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Example added - Test added
- Unused Null import removed
Co-authored-by: Copilot <[email protected]>
Brooooooklyn
force-pushed
the
fix/readable-stream-byte-mode-lock
branch
from
06ee06a to
77906bb
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces an internal Arc-based StreamState and PullController to coordinate ReadableStream pull/cancel callbacks and cleanup; updates pull/cancel callbacks to spawn async tasks and manage Arc lifetimes. Adds a new public API create_readable_stream_with_object that streams 100 StreamItem objects via a tokio mpsc channel. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as JS Test
participant RS as ReadableStream (JS)
participant Pull as PullCallback
participant State as StreamState (Arc)
participant Chan as tokio mpsc Receiver
participant Producer as Background Producer
Test->>RS: call createReadableStreamWithObject()
RS->>Chan: create tokio mpsc (tx, rx)
RS->>State: create Arc<StreamState> holding rx
RS->>Producer: spawn producer -> send 100 StreamItem
RS-->>Test: return ReadableStream
rect rgba(236,248,255,0.9)
Note over Producer,Chan: Producer sends StreamItem objects to channel
Producer->>Chan: tx.send(StreamItem)
end
Test->>RS: JS pull -> triggers PullCallback
Pull->>State: clone Arc, lock mutex, check cancelled
alt item available
State->>Chan: rx.recv()
Chan-->>Pull: StreamItem
Pull->>RS: controller.enqueue(item)
else end-of-stream
Pull->>RS: controller.close()
Pull->>State: mark end and release ownership
end
Test->>RS: call cancel()
RS->>State: cancel_callback sets cancelled, try cleanup, drop rx/producer refs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
examples/napi/src/stream.rs (2)
60-81: Prefer derivingDefaultforStreamIteminstead of a manual implAll fields on
StreamItemalready implementDefault, andFooabove uses#[derive(Default)]. You can remove the manualimpl Defaultand derive it directly on the struct to reduce boilerplate and keep things consistent.Proposed refactor
#[napi(object)] -pub struct StreamItem { +#[derive(Default)] +pub struct StreamItem { pub something: Foo, pub name: String, pub size: i32, } - -impl Default for StreamItem { - fn default() -> Self { - Self { - something: Default::default(), - name: Default::default(), - size: Default::default(), - } - } -}Based on learnings, this keeps the
#[napi(object)]pattern simple and idiomatic.
83-105: Minor nits in the object-stream example loop (_itname and panics`)The overall object-streaming logic looks good and mirrors the byte stream example, but there are two small nits:
- The loop variable
_itis actually used to setsize; the leading underscore conventionally signals “unused”. Renaming it avoids confusing linters/readers.- Using
panic!onTrySendError::{Closed, Full}will crash the thread if the receiver is dropped early. That matches the existing byte-stream example but isn’t ideal as “best practices” sample code.If you want to tweak it:
Optional refactor
- std::thread::spawn(move || { - for _it in 0..100 { + std::thread::spawn(move || { + for it in 0..100 { let item = StreamItem { something: Default::default(), name: Default::default(), - size: _it, + size: it, }; match tx.try_send(Ok(item)) { - Err(TrySendError::Closed(_)) => { - panic!("closed"); - } - Err(TrySendError::Full(_)) => { - panic!("queue is full"); - } + Err(TrySendError::Closed(_)) => { + // Receiver dropped early; just stop producing. + break; + } + Err(TrySendError::Full(_)) => { + // Should not happen with 100 items into a capacity-100 channel, + // but bail out gracefully if it does. + break; + } Ok(_) => {} } } });crates/napi/src/bindgen_runtime/js_values/stream/read.rs (1)
342-395: Commented-outtype: "bytes"increate_with_stream_bytescould use clarificationYou now omit
underlying_source.set("type", "bytes")?for the default constructor but still set it inwith_stream_bytes_and_readable_stream_class. That’s likely intentional for the locking/byte-mode behavior this PR is fixing, but the lone commented-out line is easy to misinterpret later.Consider either removing the dead line entirely or replacing it with a brief comment explaining why the plain constructor intentionally does not set
type: "bytes"while the class-based one does.examples/napi/__tests__/values.spec.ts (1)
254-259: Object-stream test covers the new API well; minor assertion message typoThe new
create readable stream from channel with objecttest nicely mirrors the byte-stream test and thoroughly checks the shape and count of emitted items, so it should catch regressions in the object streaming path.Only nit: the first assertion message has a small typo:
- t.truthy(chunk?.something, `Element ${index} doesnt have chunk.something`) + t.truthy(chunk?.something, `Element ${index} doesn't have chunk.something`)Also applies to: 2054-2075
🧹 Nitpick comments (1)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (1)
543-598: Controller-basedpullcallbacks look correct; consider validating the controller argumentSwitching
pull_callback_implto use thecontrollerargument and callenqueue/closedirectly is the right direction and matches the object-stream behavior you’re testing. One small robustness improvement:napi_get_cb_infoassumes one argument and you destructurelet [controller] = argswithout checkingargc.If this callback were ever invoked with 0 args (host bug or different embedding),
controllerwould be a nullnapi_valueandObject::from_napi_valuewould fail in a less-obvious way. You could make this fail fast and explicit:Optional guard for the controller argument
fn pull_callback_impl< T: ToNapiValue + Send + 'static, S: Stream<Item = Result<T>> + Unpin + Send + 'static, >( env: sys::napi_env, info: sys::napi_callback_info, ) -> Result<sys::napi_value> { let mut data = ptr::null_mut(); let mut argc = 1; let mut args = [ptr::null_mut(); 1]; check_status!(unsafe { sys::napi_get_cb_info( env, info, &mut argc, args.as_mut_ptr(), ptr::null_mut(), &mut data, ) }, "Get ReadableStream.pull callback info failed")?; + + if argc != 1 { + return Err(Error::new( + Status::InvalidArg, + "ReadableStream pull callback expected a single controller argument", + )); + }…and apply the same pattern to
pull_callback_impl_bytes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/napi/src/bindgen_runtime/js_values/stream/read.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/__tests__/values.spec.tsexamples/napi/example.wasi-browser.jsexamples/napi/example.wasi.cjsexamples/napi/index.cjsexamples/napi/index.d.ctsexamples/napi/src/stream.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(object)]` attribute for plain objects that should be mapped to TypeScript interfaces
Applied to files:
examples/napi/src/stream.rscrates/napi/src/bindgen_runtime/js_values/stream/read.rsexamples/napi/index.d.cts
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
crates/napi/src/bindgen_runtime/js_values/stream/read.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
Applied to files:
crates/napi/src/bindgen_runtime/js_values/stream/read.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Run `yarn workspace examples/napi build` before testing to ensure code generation reflects current Rust changes
Applied to files:
crates/napi/src/bindgen_runtime/js_values/stream/read.rs
🧬 Code graph analysis (2)
examples/napi/__tests__/values.spec.ts (1)
examples/napi/example.wasi-browser.js (2)
createReadableStreamWithObject(219-219)createReadableStreamWithObject(219-219)
examples/napi/src/stream.rs (2)
examples/napi/index.d.cts (2)
Foo(772-774)StreamItem(1117-1121)crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)
Env(59-61)new(214-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
examples/napi/index.d.cts (1)
647-648: TS declarations for object streaming look consistent with the Rust API
createReadableStreamWithObject(): ReadableStream<StreamItem>and theFoo/StreamIteminterfaces line up with the Rust#[napi(object)]definitions and the new example stream. No issues from a typings perspective.Also applies to: 772-775, 1117-1121
examples/napi/__tests__/__snapshots__/values.spec.ts.md (1)
687-688: Snapshot updates correctly mirror new type declarationsThe snapshot now includes
createReadableStreamWithObject,Foo, andStreamItemin lockstep withindex.d.cts. This keeps the type-def snapshot in sync with the generated declarations.Also applies to: 812-815, 1157-1162
examples/napi/example.wasi-browser.js (1)
217-220: WASI browser shim export forcreateReadableStreamWithObjectis wired correctlyRe-exporting
__napiModule.exports.createReadableStreamWithObjectalongside the other stream helpers is consistent with the rest of this file.examples/napi/example.wasi.cjs (1)
262-265: CJS WASI wrapper cleanly exposescreateReadableStreamWithObjectThe new export is consistent with the other
createReadableStream*bindings and should work across environments that use this WASI CJS entry.examples/napi/index.cjs (1)
726-728: New CommonJS export forcreateReadableStreamWithObjectlooks correctForwarding
nativeBinding.createReadableStreamWithObjectviamodule.exportsmatches the established pattern for other exports and aligns with the tests.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Restore type: "bytes" for byte stream methods (zero-copy BYOB optimization) - Add documentation to Foo, StreamItem structs and create_readable_stream_with_object - Fix variable naming: _it → i in stream example - Update TypeScript type definition snapshots 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eStream - Add StreamState<S> with Arc<Mutex<Option<Stream>>> + AtomicBool for safe state sharing between pull and cancel callbacks (prevents double-free and use-after-free) - Add cancel_callback<S> that sets cancelled flag and drops stream cleanly - Add PullController<T> helper to deduplicate controller binding logic - Switch from std::sync::Mutex to tokio::sync::Mutex for async compatibility - Register cancel callback on all 4 stream creation methods - Add cancellation test to verify cleanup works correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)
229-268: Memory leak: Arc strong count never cleaned up.The
Arc::into_rawat line 232 creates a raw pointer that "leaks" the Arc's strong count. However, neither callback consumes this original strong count:
pull_callback_impl(lines 742-744) callsincrement_strong_countbeforefrom_raw, creating a new referencecancel_callback(lines 705-710) callsfrom_rawthenforget, leaving the count intactThe original strong count from
into_rawis never decremented, causing theStreamStateand its contained stream to leak when the JavaScript object is garbage collected.Solution: Register a JavaScript finalizer on the underlying source object to decrement the strong count when the stream is GC'd, or restructure ownership to ensure cleanup.
🔎 Example approach using napi_add_finalizer
// After creating underlying_source, before creating the stream: extern "C" fn finalize_stream_state<S>( _env: sys::napi_env, data: *mut c_void, _hint: *mut c_void, ) { if !data.is_null() { // Consume the original Arc strong count let _ = unsafe { Arc::from_raw(data.cast::<StreamState<S>>()) }; } } unsafe { sys::napi_add_finalizer( env.raw(), underlying_source.0.value, state_ptr, Some(finalize_stream_state::<S>), ptr::null_mut(), ptr::null_mut(), ); }
755-780: Blocking lock in callback context risks stalling execution.Line 772 uses
blocking_lock()in the callback that runs after the async operation completes. This callback likely executes on the Node.js event loop thread, where blocking operations should be avoided.Furthermore, there's a potential race condition: between the cancellation check (line 748) and acquiring the lock (line 757), the stream could be cancelled. If
cancel_callbackis concurrently callingblocking_lock()(line 708), this could cause contention or unpredictable behavior.Recommendations:
- Use async cleanup in the callback since it's already in an async context
- Restructure to avoid needing the lock in the callback - the stream is already
Noneif fully consumed, or handle cleanup in a cancellation-aware manner🔎 Proposed async cleanup approach
move |env, val| { if let Some(val) = val { let enqueue_fn = controller.enqueue.borrow_back(env)?; enqueue_fn.call(val)?; } else { let close_fn = controller.close.borrow_back(env)?; close_fn.call(())?; - // Stream ended - take and drop the stream - let _ = state.stream.blocking_lock().take(); } drop(controller.enqueue); drop(controller.close); Ok(()) },The async block already handles the stream properly by returning
Nonewhen it's done, so explicit cleanup here may be redundant.
♻️ Duplicate comments (2)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)
295-365: Same memory leak pattern repeated across all constructors.All four
ReadableStreamconstructor variants (with_readable_stream_class,create_with_stream_bytes,with_stream_bytes_and_readable_stream_class) have the same memory leak issue described in the previous comment where the original Arc strong count frominto_rawis never consumed.Additionally: Consider extracting the common state creation and callback registration logic into a helper function to reduce the significant code duplication across these four constructors.
Also applies to: 370-439, 442-516
822-851: Same blocking lock issue in byte stream variant.This byte-stream variant has the identical problem as
pull_callback_impl: line 843 usesblocking_lock()in the completion callback context, which likely runs on the event loop thread and should avoid blocking operations.Apply the same fix as suggested for the non-bytes variant.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/napi/src/bindgen_runtime/js_values/stream/read.rsexamples/napi/__tests__/values.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/napi/tests/values.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (4)
2-2: LGTM: Import additions support the new async coordination pattern.The addition of
c_voidfor raw pointer handling in callbacks, the switch totokio::sync::Mutexfor async-aware locking, and theCallbackContextimport all align well with the new StreamState and cancellation infrastructure.Also applies to: 14-14, 21-21
91-91: Grammar fix applied correctly.The comment now properly reads "whether the readable stream is locked" instead of the previous grammatically incorrect version. This addresses the past review feedback.
629-644: Well-designed coordination primitive.The
StreamStatestruct effectively coordinates between pull and cancel callbacks using:
Arcfor shared ownership across callback boundariestokio::sync::Mutexfor async-safe stream accessAtomicBoolfor efficient lock-free cancellation checksThis design properly addresses the memory safety concerns mentioned in past reviews.
646-686: Excellent abstraction for controller method extraction.The
PullControllerhelper cleanly encapsulates the logic for extracting and binding theenqueueandclosemethods from the ReadableStream controller, reducing code duplication across the pull callback implementations.
… and reference leak Addresses feedback from CodeRabbit, Cursor, and Copilot: 1. **Event loop blocking (Critical)**: Replace `blocking_lock()` with `try_lock()` in cancel_callback to avoid stalling the Node.js event loop when the async pull task holds the lock. 2. **Arc memory leak**: Add `cleanup_done: AtomicBool` to StreamState with `try_claim_cleanup()` method. Uses compare_exchange to ensure exactly one callback (cancel or final pull) performs the Arc cleanup, preventing both double-free and memory leak. 3. **FunctionRef leak on error**: Restructure pull callbacks to use inner closure pattern, ensuring `controller.enqueue` and `controller.close` are always dropped even when errors occur mid-callback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)
770-778: Critical: Same undefined behavior from double Arc::from_raw.Same issue as in
cancel_callback: lines 772 and 807 both callArc::from_rawon the samedatapointer, causing undefined behavior.🔎 Proposed fix
Replace lines 770-778 with:
- let (controller, data) = PullController::<T>::from_callback_info(env, info)?; - - // Get the Arc<StreamState> - increment ref count so we don't drop the original - let state = unsafe { - Arc::increment_strong_count(data.cast::<StreamState<S>>()); - Arc::from_raw(data.cast::<StreamState<S>>()) - }; + let (controller, data) = PullController::<T>::from_callback_info(env, info)?; + + // Reconstruct the Arc from the raw pointer + let state = unsafe { Arc::from_raw(data.cast::<StreamState<S>>()) };And replace lines 802-808 with:
} else { let close_fn = controller.close.borrow_back(env)?; close_fn.call(())?; // Stream ended - try to take the stream (use try_lock to avoid blocking) if let Ok(mut guard) = state.stream.try_lock() { let _ = guard.take(); } // Try to claim cleanup responsibility for the original Arc - if state.try_claim_cleanup() { - drop(unsafe { Arc::from_raw(data.cast::<StreamState<S>>()) }); - } + // No need to do anything here - cleanup is handled by dropping state + // or by cancel_callback if cancellation happens }Then after the closure, add logic to put the Arc back if still needed:
// Always clean up FunctionRefs regardless of success/failure drop(controller.enqueue); drop(controller.close); + + // If stream is not ended and not cancelled, put Arc back for next pull + if result.is_ok() && val.is_some() && !state.cancelled.load(Ordering::SeqCst) { + let _ = Arc::into_raw(state); + } // else: let state drop to clean up + result },Actually, this needs more careful thought about the lifecycle. The cleaner fix is to use
ManuallyDropand explicit Arc cloning:let state = unsafe { Arc::from_raw(data.cast::<StreamState<S>>()) }; let state_for_async = state.clone(); // ... use state_for_async in async block ... // In the callback closure, check if we should put the Arc back: if val.is_some() && !state.cancelled.load(Ordering::SeqCst) && !state.try_claim_cleanup() { // Stream continues, put it back let _ = Arc::into_raw(state); } // else: state drops, cleaning upAlso applies to: 802-808
847-856: Critical: Same undefined behavior in bytes variant.Same double
Arc::from_rawissue as in the generic variant: lines 850 and 889 both callfrom_rawon the samedatapointer, causing undefined behavior. Apply the same fix pattern as suggested forpull_callback_impl.Also applies to: 884-890
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (7)
91-91: Grammar fix looks good.The comment now correctly reads "whether the readable stream is locked" instead of "regardless".
629-657: Well-designed shared state mechanism.The combination of
Arc,tokio::sync::Mutex, and atomic flags provides safe coordination between pull and cancel callbacks. Thetry_claim_cleanupmethod usingcompare_exchangeensures only one code path performs final cleanup, addressing previous memory leak concerns.
659-699: LGTM: Clean controller extraction and binding.The
PullControllerhelper properly extracts and binds the enqueue/close methods from the JavaScript controller, enabling later invocation from async contexts.
792-816: Excellent: Inner closure ensures FunctionRef cleanup on all paths.The inner closure pattern (lines 794-811) with explicit drops at lines 813-814 ensures
controller.enqueueandcontroller.closeare always cleaned up, even when errors occur. This properly addresses the FunctionRef leak concern from previous reviews.
874-897: Good: Same FunctionRef cleanup pattern applied consistently.The bytes variant also uses the inner closure pattern to ensure FunctionRefs are cleaned up on all code paths.
229-268: Consistent constructor pattern with proper cancel callback registration.All four ReadableStream constructors consistently:
- Create shared
StreamStateand convert to raw pointer- Register both pull and cancel callbacks with the state pointer
- Pass the underlying_source to ReadableStream constructor
The addition of cancel callback registration across all variants ensures proper cleanup on stream cancellation, addressing previous concerns about resource leaks.
Also applies to: 306-345, 379-418, 456-495
729-731: Good: Non-blocking lock attempts prevent event loop stalls.The use of
try_lock()instead ofblocking_lock()across all cleanup paths (cancel and stream end) properly addresses the previous critical concern about blocking the Node.js event loop. If the lock is held by another callback, that's fine—the other callback will see the cancellation flag and handle cleanup.Also applies to: 802-804, 884-886
- Changed cancel_callback to use single from_raw + into_raw pattern instead of increment+from_raw followed by second from_raw - Added detailed comments explaining the borrow patterns: - cancel_callback: takes ownership, puts back if not cleaning up - pull_callback: increment+from_raw for borrowing across multiple calls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Addressed CodeRabbit review comment (double Arc::from_raw)Fixed in commit d3a0c27. cancel_callback now uses the cleaner pattern suggested:
This eliminates the undefined behavior from calling pull_callback_impl keeps the increment+from_raw pattern (needed because multiple pull calls can happen before stream ends), but added detailed comments explaining why each pattern is used. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (6)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (6)
91-91: LGTM: Grammar fix applied.The documentation comment now correctly reads "whether the readable stream is locked" instead of "regardless".
230-268: LGTM: Proper state initialization and callback binding.The constructor correctly creates shared
StreamState, converts it to a raw pointer, and binds both pull and cancel callbacks with the same state pointer for proper coordination.
629-657: LGTM: Well-designed shared state coordination.The
StreamStatedesign is sound:
Mutex<Option<Pin<Box<S>>>>properly protects the stream for async accessAtomicBoolflags enable lock-free cancellation checkingtry_claim_cleanup()usingcompare_exchangeensures exactly one callback performs final Arc cleanup
659-699: LGTM: Clean abstraction for controller binding.The
PullControllerhelper properly extracts and binds the controller'senqueueandclosemethods asFunctionRefs for safe cross-boundary invocation.
765-825: LGTM: Proper borrowing pattern in pull callback.The
pull_callback_implcorrectly usesincrement_strong_count+from_rawto borrow the Arc without consuming the original reference frominto_raw. The inner closure pattern ensuresFunctionRefcleanup on all paths, and early cancellation check prevents wasted work.The cleanup coordination at lines 812-814 is sound if the race condition in
cancel_callback(flagged separately) is addressed.Note: This approval depends on fixing the potential UAF in
cancel_callbackto ensure it cannot callArc::from_raw(data)after this code has already consumed the original Arc.
851-911: LGTM: Consistent pattern for byte streams.The
pull_callback_impl_bytescorrectly mirrors the logic ofpull_callback_implwith appropriate byte-specific handling (converting toVec<u8>and wrapping inBufferSlice).
- Rename `Foo` to `NestedMetadata` for better clarity in examples - Replace manual `Default` impl with `#[derive(Default)]` for `StreamItem` - Update TypeScript definitions and snapshots 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Addressed review comments (commit 7ca9866)Fixed:
Already fixed in previous commits:
Not changed (intentional):
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
crates/napi/src/bindgen_runtime/js_values/stream/read.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/index.d.ctsexamples/napi/src/stream.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/napi/src/stream.rs
- examples/napi/tests/snapshots/values.spec.ts.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(object)]` attribute for plain objects that should be mapped to TypeScript interfaces
Applied to files:
examples/napi/index.d.cts
🧬 Code graph analysis (2)
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (2)
crates/napi/src/threadsafe_function.rs (2)
new(53-59)raw(441-443)crates/napi/src/env.rs (2)
raw(1474-1476)from_raw(72-74)
examples/napi/index.d.cts (1)
examples/napi/example.wasi-browser.js (2)
createReadableStreamWithObject(219-219)createReadableStreamWithObject(219-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (43)
- GitHub Check: ASAN - windows-latest
- GitHub Check: aarch64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: aarch64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@24 - toolchain@ stable
- GitHub Check: i686-pc-windows-msvc - node@24 - toolchain@ stable
- GitHub Check: ASAN - ubuntu-24.04
- GitHub Check: aarch64-apple-darwin - node@20 - toolchain@ stable
- GitHub Check: i686-pc-windows-msvc - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ stable
- GitHub Check: aarch64-apple-darwin - node@22 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@20 - toolchain@ stable
- GitHub Check: x86_64-unknown-linux-gnu - node@24 - toolchain@ 1.88.0
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ 1.88.0
- GitHub Check: x86_64-unknown-linux-gnu - node@22 - toolchain@ stable
- GitHub Check: x86_64-pc-windows-msvc - node@20 - toolchain@ stable
- GitHub Check: stable - armv7-unknown-linux-gnueabihf - node@22
- GitHub Check: Build only test - loongarch64-unknown-linux-gnu
- GitHub Check: Build only test - riscv64gc-unknown-linux-gnu
- GitHub Check: Test node wasi target (--cfg tokio_unstable)
- GitHub Check: build - s390x-unknown-linux-gnu
- GitHub Check: Build only test - armv7-linux-androideabi
- GitHub Check: Build only test - aarch64-unknown-linux-ohos
- GitHub Check: build-and-test-msys2 (UCRT64)
- GitHub Check: Test freebsd target
- GitHub Check: Test latest bun
- GitHub Check: Build only test - aarch64-linux-android
- GitHub Check: build - aarch64-unknown-linux-gnu
- GitHub Check: build-and-test-msys2 (MINGW64)
- GitHub Check: build-and-test-msys2 (CLANG64)
- GitHub Check: build - powerpc64le-unknown-linux-gnu
- GitHub Check: build - x86_64-unknown-linux-musl
- GitHub Check: build - x86_64-unknown-linux-gnu
- GitHub Check: Test node wasi target
- GitHub Check: build - aarch64-unknown-linux-musl
- GitHub Check: Lint SourceCode
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-apple-darwin)
- GitHub Check: Zig-Cross-Compile-On-Linux (armv7-unknown-linux-musleabihf)
- GitHub Check: Zig-Cross-Compile-On-Linux (aarch64-unknown-linux-musl)
- GitHub Check: Zig-Cross-Compile-On-Linux (x86_64-unknown-linux-musl)
- GitHub Check: Memory leak detect job
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
examples/napi/index.d.cts (1)
647-651: LGTM — New object streaming API declarations are well-formed.The new
createReadableStreamWithObject()function and supporting types (NestedMetadata,StreamItem) are correctly declared and follow established patterns in this auto-generated file. The JSDoc comments appropriately document the purpose of these additions.Also applies to: 912-915, 1122-1127
crates/napi/src/bindgen_runtime/js_values/stream/read.rs (4)
629-657: Well-designed shared state coordination.The
StreamStatestruct correctly implements the coordination pattern:
cleanup_donewithcompare_exchangeensures exactly one callback performs final cleanupSeqCstordering provides strong guarantees across the cancel/pull paths- Good documentation explaining the purpose of each field
659-699: LGTM — Clean helper abstraction for controller method binding.The
PullControllercorrectly extracts and binds theenqueue/closemethods, ensuring properthiscontext when called from Rust.
701-739: Correct ownership pattern in cancel callback.The implementation properly addresses previous review concerns:
- Single
Arc::from_rawfollowed by conditionalArc::into_rawavoids double-free UBtry_lockprevents event loop blockingtry_claim_cleanup()ensures single-owner cleanup
229-268: Consistent stream construction across all paths.All four construction methods (
new,with_readable_stream_class,create_with_stream_bytes,with_stream_bytes_and_readable_stream_class) consistently:
- Create shared
StreamStateviaArc::into_raw- Install both
pullandcancelcallbacks with the samestate_ptr- Follow the established pattern for underlying source setup
Also applies to: 306-345, 379-418, 456-495
The previous implementation had a potential use-after-free: 1. pull_callback could claim cleanup and free the Arc 2. cancel_callback could be invoked afterward with a dangling pointer 3. Arc::from_raw on freed memory = undefined behavior This fix: - Registers a invoke on underlying_source using napi_add_finalizer - The invoke frees the Arc when underlying_source is garbage collected - Both cancel_callback and pull_callback now only "borrow" using increment+from_raw pattern, never freeing the Arc directly - Removed cleanup_done AtomicBool and try_claim_cleanup (no longer needed) This ensures memory is only freed after all callbacks are guaranteed to have completed, eliminating the use-after-free race condition. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Fixed use-after-free vulnerability (commit 8cae6b1)Cursor correctly identified a potential use-after-free in The bug:
The fix:
This ensures the Arc is only freed after all callbacks are guaranteed to have completed (when the JS object is GC'd), eliminating the race condition entirely. |
The callback closure after async work completes now re-checks the cancelled flag before calling enqueue. This prevents enqueueing items if cancel was called while stream.next().await was in progress. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
2 participants
Note
Introduces object-item streaming and overhauls
ReadableStreaminternals for safe lifecycle and cleanup.Arc<StreamState>(Mutex-guarded stream +AtomicBool cancelled); addscancelcallback, registers finalizer viaregister_invoketo free state on GC, and updates pull paths to use controllerenqueue/closewith async locking and cancellation checkspullimplementationNestedMetadataandStreamItemtypes and newcreateReadableStreamWithObject()export in examples (browser/Node WASI shims, type defs, snapshots)locked()doc commentWritten by Cursor Bugbot for commit 157dff4. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.