Revert "feat: revert internal inspector changes (#1225)" #1253
Conversation
This reverts commit a45a827.
WalkthroughThe PR replaces the in-memory InspectorSessionProxy flow with a remote IO session model. It introduces NewSessionTxMsg and RemoteInspectorSessionSend, and adds a public InspectorIoDelegate (new_session_tx, deregister_rx). JsRuntimeInspector, SessionContainer, InspectorSession, and related lifecycle/polling logic are reworked to manage Unbounded NewSessionTxMsg channels, IO session futures, and remote session pumping. core/lib.rs now exports InspectorIoDelegate and RemoteInspectorSessionSend; InspectorSessionProxy exports are removed. dcore's InspectorServer/InspectorInfo now use InspectorIoDelegate for session initiation and deregistration. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI 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). (11)
🔇 Additional comments (3)
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: 1
🧹 Nitpick comments (2)
core/inspector.rs (2)
43-48: TODO comment indicates incomplete work.The
NewSessionTxMsgtype alias has a TODO to remove it. This suggests the type is meant to be temporary or refactored further. Consider addressing this before merging or creating a tracking issue.Would you like me to open an issue to track this cleanup?
185-195: Silent failure intry_poll_sessions.When
try_borrow_mut()fails, the function silently returns without any indication. This could mask issues where sessions aren't being polled when expected. Consider adding a debug log or metric for visibility.pub fn try_poll_sessions(&self) { let Ok(mut sessions) = self.sessions.try_borrow_mut() else { + #[cfg(debug_assertions)] + eprintln!("try_poll_sessions: sessions already borrowed"); return; }; self.poll_sessions_inner(&mut sessions, None); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/inspector.rs(22 hunks)core/lib.rs(1 hunks)dcore/src/inspector_server.rs(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/lib.rs (2)
core/runtime/tests/misc.rs (1)
inspector(433-441)core/runtime/jsruntime.rs (2)
inspector(1306-1308)inspector(2472-2474)
⏰ 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). (12)
- GitHub Check: build-macos / macos test
- GitHub Check: build-windows / windows lint
- GitHub Check: build-linux-arm / linux-arm test
- GitHub Check: build-macos / macos lint
- GitHub Check: build-windows / windows test
- GitHub Check: build-linux-arm / linux-arm test-miri
- GitHub Check: build-linux / linux test-miri
- GitHub Check: build-linux-arm / linux-arm lint
- GitHub Check: build-linux / linux test-ops
- GitHub Check: build-linux / linux test
- GitHub Check: build-linux / linux lint-deps
- GitHub Check: build-linux / linux coverage
🔇 Additional comments (11)
core/inspector.rs (5)
62-87: NewInspectorIoDelegatepublic API looks reasonable.The struct provides a clean interface for remote session management with
new_remote_sessionandpoll_deregister_rx. However, the TODO at line 78-79 notes thatpoll_deregister_rxis an antipattern and should be handled inDrop. This is a valid concern for resource cleanup.Consider addressing the TODO about implementing proper cleanup in
Dropto avoid the polling antipattern.
309-320: Initial pump and waker setup after inspector creation.This block sets up the initial waker state and performs an initial poll. The pattern looks correct - it ensures the waker is properly registered before returning the inspector. The raw pointer manipulation for
state_ptris safe here sincestateis owned by theRcwhich lives as long as the inspector.
419-428: TODO comment questions session selection logic.The comment at line 419 asks "why the first random session?" - this suggests uncertainty about the correctness of selecting an arbitrary session for
break_on_next_statement. If multiple local sessions exist, breaking on only one might be unexpected behavior.Is it intentional to only break on the first session when multiple local sessions exist? This could lead to confusing debugger behavior.
590-616:pump_messages_for_remote_sessionspolling loop structure.The loop correctly handles both new session registration and pumping existing sessions. However, the TODO at line 604-606 raises a valid question about whether to fully drain the new session queue before polling existing sessions. The current behavior (immediately continuing on new session) seems reasonable for responsiveness.
653-658: Interrupt handler uses raw pointer to inspector state.The
handle_interruptfunction casts a raw pointer toJsRuntimeInspectorState. This is safe as documented because the waker's lifetime is tied toJsRuntimeInspector, but it requires careful maintenance. The safety comment is appreciated.dcore/src/inspector_server.rs (5)
93-97: Inspector registration now uses IO delegate pattern.The change from direct session sender/receiver handling to
create_io_delegate()simplifies the registration flow. TheArc<InspectorIoDelegate>is properly passed toInspectorInfo.
179-192: WebSocket session creation using new remote session API.The session creation flow is clean - a callback closure captures
outbound_txand sends inspector messages to the websocket. Theeprintln!debug message at line 182 is helpful for debugging.The TODO at line 191 raises a valid concern about whether the inspector is properly deregistered from
JsRuntimewhen the session ends. This ties into the session cleanup issue noted ininspector.rs.Verify that session cleanup propagates correctly when the WebSocket disconnects. The TODO suggests this may not be fully implemented.
247-254: Deregistration polling retains inspectors until deregistered.The
poll_deregister_rxapproach works but is flagged as an antipattern in the source file. The logic correctly removes inspectors from the map when their deregister signal fires.
274-291: Refactored async loop uses boxed futures.The change from
pin!()toboxed_local()forshutdown_rx_futandaccept_futis a reasonable approach. Both patterns work, but boxed futures have a small allocation overhead. Since this is a server accept loop, the overhead is negligible.
434-458: InspectorInfo struct updated with Arc.The struct now holds
io_delegate: Arc<InspectorIoDelegate>instead of separate sender/receiver channels. This is a cleaner abstraction that encapsulates the session creation logic.core/lib.rs (1)
89-96: Public API additions for inspector IO delegate.The new exports
InspectorIoDelegateandRemoteInspectorSessionSendare added to expose refactored inspector session management. Verify that no external consumers depend onInspectorSessionProxybefore merging, as removing this export would constitute a breaking change to the public API.
| async fn pump_inspector_session_messages( | ||
| session: Rc<InspectorSession>, | ||
| mut rx: UnboundedReceiver<String>, | ||
| ) { | ||
| while let Some(msg) = rx.next().await { | ||
| session.dispatch_message(msg); | ||
| } | ||
| // TODO(bartlomieju): notify that the IO has disconnected | ||
| // and the session should be dropped | ||
| } |
There was a problem hiding this comment.
TODO: Remote session cleanup on disconnect.
The TODO at lines 806-807 indicates that session cleanup on IO disconnect is not fully implemented. When the pump_inspector_session_messages future completes (channel closed), the session should be removed from SessionContainer.local. Currently, this appears to leak the session entry.
When the remote session's message channel closes, the session remains in SessionContainer.local but is no longer functional. This could cause:
- Memory leaks for long-running processes
- Incorrect
sessions_state()reporting
Consider implementing proper cleanup:
async fn pump_inspector_session_messages(
session: Rc<InspectorSession>,
mut rx: UnboundedReceiver<String>,
-) {
+) -> i32 {
while let Some(msg) = rx.next().await {
session.dispatch_message(msg);
}
- // TODO(bartlomieju): notify that the IO has disconnected
- // and the session should be dropped
+ session.id
}Then handle the returned session ID in pump_messages_for_remote_sessions to remove the session from local.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In core/inspector.rs around lines 799-808, the pump_inspector_session_messages
future currently drops out when the UnboundedReceiver closes but does not remove
the session from SessionContainer.local, leaking entries and stale state; fix by
obtaining the session id (clone or call session.id()) before the message loop,
and when the rx loop ends call into the SessionContainer to remove that id from
local (acquire the same lock used elsewhere and remove by id), then change
pump_messages_for_remote_sessions to accept and handle the returned/removed
session id so the container state is consistent and the session is fully cleaned
up.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
- Coverage 81.43% 80.13% -1.31%
==========================================
Files 97 105 +8
Lines 23877 27570 +3693
==========================================
+ Hits 19445 22094 +2649
- Misses 4432 5476 +1044 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // However it is can happen that poll_sessions() gets re-entered, e.g. | ||
| // when an interrupt request is honored while the inspector future is polled | ||
| // by the task executor. We let the caller know by returning some error. | ||
| pub fn try_poll_sessions(&self) { |
There was a problem hiding this comment.
This is now segfaulting in Deno - try_borrow_mut raises:
Exception Type: EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000018
Exception Codes: 0x0000000000000001, 0x0000000000000018
Termination Reason: Namespace SIGNAL, Code 11 Segmentation fault: 11
Terminating Process: exc handler [5185]
VM Region Info: 0x18 is not in any region. Bytes before following region: 4343693288
REGION TYPE START - END [ VSIZE] PRT/MAX SHRMOD REGION DETAIL
UNUSED SPACE AT START
--->
__TEXT 102e78000-112b30000 [252.7M] r-x/r-x SM=COW /Users/USER/*/deno
Thread 0 Crashed:: main Dispatch queue: com.apple.main-thread
0 deno 0x10b4fa8cc core::cell::RefCell$LT$T$GT$::try_borrow_mut::h1c6c30c81af95cae + 40
1 deno 0x10b6da628 deno_core::inspector::JsRuntimeInspectorState::try_poll_sessions::hb4971fbdf9e5066d + 44
2 deno 0x10b6dd850 deno_core::inspector::handle_interrupt::h2bc97e9d12cf0227 + 92
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/inspector.rs (1)
796-805: Session cleanup still needed.The TODO at lines 803-804 about notifying when IO disconnects is already flagged in past review comments. This remains a valid concern for preventing session leaks.
🧹 Nitpick comments (4)
core/inspector.rs (4)
78-86: TODO: Consider lifecycle management for deregister_rx.The TODO correctly identifies that polling the deregister receiver manually is an antipattern. This should ideally be handled automatically through Drop or a background task. However, since this is explicitly marked by the original author as a known design issue, it's acceptable for this PR.
419-424: Consider documenting session selection logic.The TODO at line 419 questions why the first session is selected. If this is intentional behavior (e.g., any session can trigger the debugger), consider documenting this in a comment. If specific session selection is needed, clarify the requirements.
590-616: Consider session queue draining strategy.The TODO at lines 604-605 raises a valid question about whether to fully drain
new_io_session_rxbefore polling existing sessions. The current interleaved approach provides fairness. If you need to prioritize new session creation for responsiveness, consider fully draining the queue first.
669-676: Document interrupt handling logic.The TODO at lines 669-670 suggests there's complex interaction logic that should be documented. Consider adding detailed comments explaining the interrupt request handling and state_ptr lifecycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/inspector.rs(24 hunks)
⏰ 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). (13)
- GitHub Check: build-linux / linux test
- GitHub Check: build-linux / linux lint-deps
- GitHub Check: build-windows / windows lint
- GitHub Check: build-linux / linux coverage
- GitHub Check: build-linux-arm / linux-arm test
- GitHub Check: build-linux / linux test-miri
- GitHub Check: build-linux / linux test-publish
- GitHub Check: build-linux-arm / linux-arm test-miri
- GitHub Check: build-macos / macos lint
- GitHub Check: build-windows / windows test
- GitHub Check: build-linux-arm / linux-arm lint
- GitHub Check: build-macos / macos test
- GitHub Check: build-linux / linux test-ops
🔇 Additional comments (5)
core/inspector.rs (5)
431-443: LGTM.The single delegate constraint is properly enforced, and the initialization logic is correct.
653-658: LGTM after fixing line 316-317.The interrupt handler correctly uses the state pointer and handles borrow failures gracefully. Once the critical pointer bug at lines 316-317 is fixed, this will be safe.
518-542: LGTM.The session state logic correctly handles the empty case and properly checks session kinds.
570-588: LGTM.The
create_new_sessionmethod correctly manages session IDs and insertion into the local map.
478-506: LGTM.The SessionContainer field updates properly support the new remote IO session model with appropriate channel and futures management.
2 participants
This reverts commit a45a827.