fix(ext/node): http client compat improvements#33337
Merged
Merged
Conversation
Remove the overly broad `req.destroyed` early-return guard in socketCloseListener. When req.abort() is called, req.destroyed is set to true before the socket 'close' event fires — causing socketCloseListener to skip res.destroy(), so 'aborted' and 'error' events never reached the response. Node.js does not have this guard; it sets req.destroyed inside socketCloseListener itself. The original Deno guard was a workaround for a timing issue with responseKeepAlive, but the !req check alone is sufficient. Enables test-http-aborted.js in the compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
In Node.js, process.nextTick always runs before I/O callbacks. The HTTP client uses nextTick to defer emitFreeNT (which returns sockets to the agent's free pool). In Deno, I/O can interleave with nextTick, so a server FIN may arrive before emitFreeNT runs, making the socket non-writable. Fix by checking socket.destroyed instead of !socket.writable in the agent's 'free' handler. A socket that received a server FIN but hasn't been destroyed yet can still be pooled — it will be cleaned up by the 'close' listener installed in installListeners. Enables test-http-agent-keepalive.js in the compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The globalAgent was exported as a static value, so assigning http.globalAgent = newAgent had no effect on subsequent requests. Fix by: - Making globalAgent a mutable let export in _http_agent.js - Adding setGlobalAgent() helper - Using getter/setter on the http default export - Accessing globalAgent via httpAgent.globalAgent in _http_client.js (dynamic lookup instead of static import binding) Enables test-http-client-override-global-agent.js in compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Node.js spreads the URL object (...url) into the options so that user-added properties (like headers) are preserved. Deno only extracted known URL properties, losing any user extensions. Enables test-http-client-request-options.js in compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
When the server emits 'connect' or 'upgrade', the socket should be fully detached from the HTTP server — all server-added listeners removed. Previously Deno left them attached, causing listener count assertions to fail and the socket to remain under server control. Match Node.js behavior by removing data, end, close, drain, error, and timeout listeners before emitting the event. Also remove the Deno-specific ConnectionsList tracking listener and pop the socket from the connections list. Enables test-http-connect.js and test-http-connect-req-res.js in the compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Add strict content-length validation to OutgoingMessage: - In write_(): throw if bytes written exceed content-length (or don't match on end) - In end(): throw if total bytes written doesn't match content-length - Add ERR_HTTP_CONTENT_LENGTH_MISMATCH error class Enables test-http-content-length-mismatch.js in compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ket guard - Implement ERR_HTTP_CONTENT_LENGTH_MISMATCH validation in write_() and end() for strictContentLength mode - Tighten agent free socket guard: use socket.destroyed for early exit, but still check !socket.writable before assigning to queued requests (prevents handing half-closed sockets to new requests) Enables test-http-content-length-mismatch.js in compat suite. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several fixes to
node:httpclient and server compatibility:req.abort()- removed overly broadreq.destroyedguard insocketCloseListenerthat prevented cleanup of the response streamsocket.destroyedfor early exit in agent's'free'handler, but still check!socket.writablebefore assigning to queued requests (prevents handing half-closed sockets to new requests)http.globalAgentassignable -globalAgentwas a static import binding; now uses getter/setter sohttp.globalAgent = newAgenttakes effect on subsequent requestsurlToHttpOptions- match Node's...urlspread so user-added properties (likeheaders) on URL objects are preserved'connect'/'upgrade'event, matching Node's behaviorERR_HTTP_CONTENT_LENGTH_MISMATCH- add strict content-length validation inwrite_()andend()whenstrictContentLengthis enabledNew compat tests enabled
test-http-aborted.jstest-http-agent-keepalive.jstest-http-client-override-global-agent.jstest-http-client-request-options.jstest-http-client-set-timeout.jstest-http-client-timeout-event.jstest-http-connect-req-res.jstest-http-connect.jstest-http-content-length-mismatch.jsTest plan
./x test-node httppasses (5 tests)./x lintand./x fmtpass🤖 Generated with Claude Code