deno
472d0069 - fix(ext/node): capture IPC handle eagerly to fix cluster send deadlock (#34661)

Commit
114 days ago
fix(ext/node): capture IPC handle eagerly to fix cluster send deadlock (#34661) ## Problem `node_compat::parallel::test-cluster-send-deadlock.js` was flaky in CI and was disabled in #34325. The most visible symptom was: ``` Not implemented: ChildProcess.send with non-TCP net.Socket handle ``` (see denoland/deno#34328, originally tracked as denoland/deno#34180). ## Root cause (the `notImplemented` crash) `ChildProcess.prototype.send` derived a handle's IPC info (`getIpcHandleInfo`) **lazily**, only when the message was about to be written. The IPC protocol keeps a single handle in flight at a time: while one handle send awaits its `NODE_HANDLE_ACK`, subsequent sends are pushed onto `handleQueue` and only serialized later, when the queue drains on ACK. That deferral races against the socket being torn down. The primary accepts two connections and hands each to a worker that immediately `end()`s them. The second `worker.send('handle', socket)` gets queued behind the first handle's ACK; by the time it drains, the server socket has been destroyed and its `_handle` is `null`. `getIpcHandleInfo` then hits `null instanceof TCP === false` and throws `notImplemented`, crashing the worker. ## Fixes in this PR Two IPC handle-passing correctness fixes, both also exercised by the cluster/child_process handle-passing tests that already run in CI: 1. **Derive the handle's IPC info eagerly** in `send()`, while the socket is still alive, *before* deciding whether the send must be queued. `fdForIpc()` dups the underlying fd, so the captured copy outlives the original socket's destruction and the deferred write still sends a valid descriptor. The write/queue logic is factored out of `send()` into `enqueueOrDispatch` (queue-or-write) and `dispatch` (write + ACK/error handling), so queued items carry their already-derived `handleInfo`. A genuinely unsupported handle now throws synchronously from `send()` (matching Node) rather than later at drain time. 2. **Release held handles when the channel disconnects.** A handle send that already wrote keeps its local copy open until `NODE_HANDLE_ACK` arrives; queued sends aren't written yet. On teardown (disconnect / peer CLOSE / read error) that ACK never comes, so those `closeAfterSend` handles — live `net.Socket`s materialized with `readable: true`, holding a ref'd read — leaked and kept the event loop alive. `cleanupPendingHandles()` releases them from `disconnect()` and the read loop's teardown path. ## Test status — still disabled With these fixes the test passes reliably on release builds and on every platform I could test locally. However, it **still times out intermittently (10s) on debug Linux** due to a mutual handle-send / disconnect race in the cluster IPC protocol (the worker forwards its sockets back to the primary, then disconnects; the disconnect handshake is queued behind a reply handle awaiting an ACK). I could not reproduce this outside debug Linux despite 400+ local runs (forcing repro + CPU-saturated parallelism on macOS), so I have **kept the test disabled** rather than flake CI. The remaining flakiness stays tracked in denoland/deno#34180. So this PR does **not** re-enable the test / close the flakiness issue; it lands the two runtime correctness improvements and documents the residual debug-Linux race. ## Testing - All config-listed cluster/child_process handle-passing tests pass: `test-cluster-concurrent-disconnect`, `test-cluster-net-send`, `test-cluster-rr-*`, `test-cluster-send-handle-twice`, `test-cluster-send-socket-to-worker-http-server`, `test-cluster-shared-*`, `test-child-process-send-*`, `test-child-process-ipc*`, etc. - `test-cluster-send-deadlock.js` itself: passes 50/50 locally, but remains disabled because of the debug-Linux timeout described above. --------- Co-authored-by: divybot <divybot@users.noreply.github.com> Co-authored-by: Divy Srivastava <me@littledivy.com>
Author
Parents
Loading