fix(ext/node): cancelling Readable.toWeb(req) no longer destroys the socket#33570
Conversation
…socket The HTTP rewrite (#33208) ported Node.js's IncomingMessage._destroy(), which destroys the underlying socket when the request body is not fully consumed. However, the webstreams adapter's cancel() callback called destroy() directly, bypassing the destroyer() helper that Node.js uses. In Node.js, destroyer() detects server requests (via isServerRequest()) and nullifies the socket reference before calling destroy(), so _destroy() skips socket teardown. This keeps the connection alive for the response. Fixes #33567
The client body stream's pull() creates a setTimeout that may still be pending when the stream is cancelled. Track and clear all pending timers in the cancel() callback so Deno's test sanitizer does not flag a leak.
lunadogbot
left a comment
lunadogbot
left a comment
There was a problem hiding this comment.
LGTM — verified the fix lines up with Node's lib/internal/webstreams/adapters.js, where the readable-adapter cancel callback also goes through destroyer(). Walked the chain: web-stream cancel → destroyer(req, reason) → isServerRequest(req) is true (IncomingMessage has _consuming and _dumped per internal/streams/utils.js:288) → req.socket = null then req.destroy(err), so the existing _destroy body in our _http_server.js no longer tears down the connection. Independently verified that req.socket = null does not affect res.socket, since ServerResponse.assignSocket sets res.socket separately (_http_server.js:308). The regression test asserts both status 200 and body OK, which can only succeed if the socket survived the cancel; reverting line 562 of the polyfill would fail that assertion.
2 participants
Fixes #33567
Summary
IncomingMessage._destroy(), which destroys the underlying socket when the request body is not fully consumed. The webstreams adapter'scancel()callback calleddestroy()directly, bypassing thedestroyer()helper that Node.js uses.destroyer()detects server requests (viaisServerRequest()) and nullifies the socket reference before callingdestroy(), so_destroy()skips socket teardown. This keeps the connection alive for the response.destroyer()instead ofdestroy()innewReadableStreamFromStreamReadable's cancel callback, matching Node.js's behavior.Test plan
tests/unit_node/http_test.tsthat creates an HTTP server, converts the request to a web ReadableStream viaReadable.toWeb(), cancels the reader, then verifies the response can still be sent successfully.