inspector: load network tracking modules eagerly - #64379
Conversation
Sorry, something went wrong.
`Network.enable` is dispatched to the main thread through a V8 interrupt that can run in the middle of arbitrary JavaScript execution, including while another module is still being loaded by require(). The network tracking enable()/disable() helpers lazily require()d their modules, which pull in `inspector`, `worker_threads` and `stream`. When the interrupt landed mid-require(), require() returned a half-initialized module and threw (for example "Class extends value undefined" from internal/worker/io, or "require(...).enable is not a function"). The inspector agent treats any exception thrown while toggling network tracking as unrecoverable and aborts the whole process. Load the three network tracking modules eagerly at setup time so that enable()/disable() never call require() from an interrupt. They now run during setupNetworkInspection() at bootstrap, before the inspector can dispatch Network.enable. Fixes: nodejs#64308
|
Review requested:
|
Sorry, something went wrong.
|
@legendecas mind taking a look or kicking off CI when you have a moment? Open ~4 weeks with no CI run yet — small, tested fix for #64308. |
Sorry, something went wrong.
legendecas
left a comment
There was a problem hiding this comment.
Related to #65028, I think this does not fully fix the issue of calling into JS during an interrupt. An alternative could be loading the tracking module when --experimental-network-inspection is set at startup, and enable it immediately.
Sorry, something went wrong.
|
Thanks @legendecas. You're right: #65028 changes the async hook toggle, but not ToggleNetworkTracking, so #64308 can still call into JS from the interrupt. This is only a partial fix. Your alternative works for me: with --experimental-network-inspection, load and enable tracking at startup so Network.enable doesn't toggle anything. I can rework the PR this way, or make ToggleNetworkTracking interrupt-aware on top of #65028. Which do you prefer? |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Summary
--experimental-network-inspection can abort the whole Node.js process when a debugger attaches. Reported in #64308.
Network.enable is delivered to the main thread via a V8 interrupt that can run in the middle of arbitrary JavaScript execution — including while another module is still being loaded by require(). The network-tracking enable()/disable() helpers lazily require()d their modules (network_http, network_http2, network_undici), which transitively pull in inspector → worker_threads → internal/worker/io → stream. When the interrupt lands mid-require(), require() returns a half-initialized module and throws — for example Class extends value undefined from internal/worker/io, or require(...).enable is not a function. Agent::ToggleNetworkTracking (src/inspector_agent.cc) treats any exception thrown while toggling network tracking as unrecoverable and calls UNREACHABLE(), aborting the process.
Fix
Load the three network-tracking modules eagerly at module scope, so enable()/disable() never call require() from an interrupt. The requires now run during setupNetworkInspection() at bootstrap, before the inspector can dispatch Network.enable. This only affects the opt-in --experimental-network-inspection flag, where an inspector is already active.
Verification
Minimal repro from the issue:
node --inspect-wait --experimental-network-inspection -e "require('net')"then attach a debugger and send Network.enable.
Driving the real inspector path locally (attach over WebSocket, Runtime.runIfWaitingForDebugger, then Network.enable with jitter to spray the interrupt across the require window):
Adds test/parallel/test-inspector-network-tracking-eager-load.js, which asserts the network-tracking modules are loaded at setup and that enable()/disable() do not lazily load any module.
Note
A deeper hardening — not calling UNREACHABLE() in ToggleNetworkTracking when the toggle callback throws — would defend against this class of failure at the C++ layer as well. I kept this PR to the minimal JS change that fixes the reported crash, but I'm happy to add that as a follow-up if maintainers prefer.
Fixes: #64308