feat(node/http): enable keepAlive connection reuse for HTTP Agent#31709
feat(node/http): enable keepAlive connection reuse for HTTP Agent#31709fraidev merged 5 commits intodenoland:mainfrom
Conversation
b769190 to
e6c6fc1
Compare
|
Can we pass some node_compat tests in https://github.com/search?q=repo%3Adenoland%2Fnode_test+agent&type=code |
|
It would be great if we could confirm whether this repro is getting faster. |
Yeah, |
I couldn't replicate this, seems that is not slow now, even with the latest Deno. |
Changes SummaryThis PR implements HTTP Agent keepAlive connection reuse for Node.js HTTP clients, enabling socket pooling and reuse across multiple requests. The changes reduce latency from ~626ms to ~164ms by avoiding new socket creation for each request, bringing Deno's performance in line with Node.js. Type: feature Components Affected: ext/node/ops/http.rs, ext/node/polyfills/http.ts, ext/node/polyfills/internal/streams/utils.js, Node.js HTTP Agent implementation, HTTP connection pooling Files Changed
Architecture Impact
Risk Areas: Socket state mutation: Complex manipulation of socket._readableState and _writableState flags could cause unexpected behavior if state flags are misaligned, Resource lifecycle: Socket reuse requires careful handling of resource cleanup and attachment to prevent premature destruction or memory leaks, Upgrade request detection: WebSocket upgrade detection via header matching must correctly distinguish upgrade requests from normal keepAlive requests, Error handling: Stale connection errors are remapped to ECONNRESET - incorrect remapping could break client retry logic, TLS socket reuse: Complex interaction between TLS and TCP socket reuse with encrypted flag tracking, Resource leaks: If socket reclamation fails (e.g., receiver dropped), socket resources might leak or become orphaned, Platform-specific code paths: Different handling for Unix sockets, Vsock, and Tunnel types - untested code paths could fail silently Suggestions
Full review in progress... | Powered by diffray |
|
|
||
| #[op2(async)] | ||
| #[serde] | ||
| pub async fn op_node_http_fetch_response_upgrade( |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
|
||
| // It's possible that the socket will be destroyed, and removed from | ||
| // any messages, before ever calling this. In that case, just skip | ||
| // it, since something else is destroying this connection anyway. |
There was a problem hiding this comment.
🟡 MEDIUM - Logic error: always-true condition in _destroy
Agent: bugs
Category: bug
Description:
Line 1263 sets this.complete = true, then line 1264 checks !this.readableEnded || !this.complete. Since complete is always true at this point, !this.complete is always false. The condition reduces to just !this.readableEnded.
Suggestion:
Review the logic - if the intent is to check if response was prematurely terminated, the check should probably occur before setting complete=true, or the condition should use &&.
Confidence: 75%
Rule: qual_inverted_logic_js
Review ID: 8e3324f7-5f2b-4f78-b80d-469377cc6e2b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| } catch (_e) { | ||
| // Socket reuse is best-effort. | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Error suppression without logging in socket reuse
Agent: react
Category: quality
Description:
The catch block catches all errors with comment 'Socket reuse is best-effort' but provides no logging. While intentional, errors here could indicate socket pooling issues.
Suggestion:
Add debug logging: catch (_e) { debug('socket_reuse_failed', _e); /* best-effort */ }
Confidence: 65%
Rule: ts_log_errors_instead_of_failing_silently
Review ID: 8e3324f7-5f2b-4f78-b80d-469377cc6e2b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| /// Returns the socket after the HTTP response body has been fully consumed. | ||
| /// This enables keepAlive connection reuse for the Node.js HTTP Agent. | ||
| /// Returns the new resource ID for the socket, or None if the connection | ||
| /// cannot be reused (e.g., connection error or already retrieved). |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| core.read(this._bodyRid, buf).then(async (bytesRead) => { | ||
| if (bytesRead === 0) { | ||
| // Return the socket to the agent pool BEFORE pushing null. | ||
| // This must happen before the stream ends, otherwise the socket | ||
| // may already be marked as destroyed. | ||
| await this.#tryReturnSocket(); | ||
| this.push(null); | ||
| } else { | ||
| this.push(Buffer.from(buf.subarray(0, bytesRead))); | ||
| } | ||
| }); | ||
| } |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @@ -543,8 +555,13 @@ class ClientRequest extends OutgoingMessage { | |||
| // Simulates "secure" event on TLSSocket | |||
| // This makes yarn v1's https client working | |||
| this.socket.authorized = true; | |||
| // Mark the socket as encrypted for keepAlive reuse detection | |||
| this.socket.encrypted = true; | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| async #tryReturnSocket() { | ||
| const socket = this.socket; | ||
| if (!socket) { | ||
| return; | ||
| } | ||
|
|
||
| const req = this.req; | ||
| // Only pool the socket if keepAlive is enabled. | ||
| if (!req?.shouldKeepAlive) { | ||
| return; | ||
| } | ||
|
|
||
| const handle = req?._socketHandle || socket._handle; | ||
| if (!handle) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const newRid = await op_node_http_response_reclaim_conn(this._bodyRid); | ||
| if (newRid == null) { | ||
| return; | ||
| } | ||
|
|
||
| const remoteAddr = { | ||
| hostname: socket.remoteAddress || "", | ||
| port: socket.remotePort || 0, | ||
| }; | ||
| const localAddr = { | ||
| hostname: socket.localAddress || "", | ||
| port: socket.localPort || 0, | ||
| }; | ||
|
|
||
| const isTls = socket.encrypted === true; | ||
| const conn = isTls | ||
| ? new TlsConn(newRid, remoteAddr, localAddr) | ||
| : new TcpConn(newRid, remoteAddr, localAddr); | ||
|
|
||
| handle[kStreamBaseField] = conn; | ||
| if (!socket._handle) { | ||
| socket._handle = handle; | ||
| } | ||
|
|
||
| // Reset socket state for reuse. | ||
| if (socket._readableState?.[kState] !== undefined) { | ||
| socket._readableState[kState] &= ~(kDestroyed | kErrored); | ||
| } | ||
| if (socket._writableState?.[kState] !== undefined) { | ||
| socket._writableState[kState] &= | ||
| ~(kEnding | kEnded | kDestroyed | kErrored); | ||
| socket._writableState.writable = true; | ||
| } | ||
| handle.destroyed = false; | ||
|
|
||
| // Agent's 'free' handler checks socket._httpMessage.shouldKeepAlive. | ||
| if (req && !socket._httpMessage) { | ||
| socket._httpMessage = req; | ||
| } | ||
|
|
||
| // Remove error listener to prevent accumulation on reused sockets. | ||
| if (req?._socketErrorListener) { | ||
| socket.removeListener("error", req._socketErrorListener); | ||
| req._socketErrorListener = null; | ||
| } | ||
|
|
||
| socket.emit("free"); | ||
|
|
||
| // Clear references so old request/response don't destroy the pooled socket. | ||
| if (req) { | ||
| req.socket = null; | ||
| } | ||
| this.socket = null; | ||
| } catch (_e) { | ||
| // Socket reuse is best-effort. | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM - Private method #tryReturnSocket lacks unit test coverage
Agent: testing
Category: quality
Description:
New private async method '#tryReturnSocket()' handles complex socket state reset including stream state bit manipulation and handle restoration. The code is complex (lines 1226-1244) and would benefit from focused unit tests.
Suggestion:
Add unit tests verifying socket state flags properly reset and error handling when reclaim fails
Confidence: 72%
Rule: test_new_parameter_coverage
Review ID: 8e3324f7-5f2b-4f78-b80d-469377cc6e2b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
|
|
||
| // Stop reading and save handle for keepAlive restoration. | ||
| this.socket?._handle?.readStop(); | ||
| this._socketHandle = this.socket?._handle; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| }, | ||
| }); | ||
|
|
||
| Deno.test("[node/https] request directly with key and cert as agent", async () => { |
There was a problem hiding this comment.
🟠 HIGH - Double type assertion 'as never as Type'
Agent: typescript
Category: bug
Description:
Using 'as never as net.AddressInfo' is a double assertion that completely bypasses TypeScript's type safety. This pattern is a code smell even if it works.
Suggestion:
Remove 'as never' and use proper type handling: (server.address() as net.AddressInfo | null)?.port with null check
Confidence: 80%
Rule: ts_type_assertion_abuse
Review ID: 8e3324f7-5f2b-4f78-b80d-469377cc6e2b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
| // Test HTTP Agent keepAlive connection reuse | ||
| import http from "node:http"; | ||
| import { Agent } from "node:http"; | ||
| import { strictEqual } from "node:assert"; | ||
|
|
||
| const agent = new Agent({ | ||
| keepAlive: true, | ||
| maxSockets: 1, | ||
| maxFreeSockets: 1, | ||
| }); | ||
|
|
||
| const server = http.createServer((_req, res) => { | ||
| res.end("ok"); | ||
| }); | ||
|
|
||
| function makeRequest(path: string): Promise<{ reusedSocket: boolean }> { | ||
| return new Promise((resolve, reject) => { | ||
| const req = http.get( | ||
| { | ||
| host: "localhost", | ||
| port: (server.address() as { port: number }).port, | ||
| agent, | ||
| path, | ||
| }, | ||
| (res) => { | ||
| const reusedSocket = req.reusedSocket; | ||
| res.on("data", () => {}); | ||
| res.on("end", () => { | ||
| resolve({ reusedSocket }); | ||
| }); | ||
| }, | ||
| ); | ||
| req.on("error", reject); | ||
| }); | ||
| } | ||
|
|
||
| function waitForFreeSockets(): Promise<void> { | ||
| return new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } | ||
|
|
||
| server.listen(0, async () => { | ||
| const name = `localhost:${(server.address() as { port: number }).port}:`; | ||
|
|
||
| // First request - should create new socket | ||
| const first = await makeRequest("/first"); | ||
| strictEqual( | ||
| first.reusedSocket, | ||
| false, | ||
| "First request should not reuse socket", | ||
| ); | ||
| console.log("First request: reusedSocket =", first.reusedSocket); | ||
|
|
||
| // Wait for socket to be returned to pool | ||
| await waitForFreeSockets(); | ||
| strictEqual( | ||
| agent.freeSockets[name]?.length, | ||
| 1, | ||
| "Socket should be in freeSockets pool", | ||
| ); | ||
| console.log("Socket in freeSockets pool:", agent.freeSockets[name]?.length); | ||
|
|
||
| // Second request - should reuse socket | ||
| const second = await makeRequest("/second"); | ||
| strictEqual(second.reusedSocket, true, "Second request should reuse socket"); | ||
| console.log("Second request: reusedSocket =", second.reusedSocket); | ||
|
|
||
| // Wait for socket to be returned to pool | ||
| await waitForFreeSockets(); | ||
| strictEqual( | ||
| agent.freeSockets[name]?.length, | ||
| 1, | ||
| "Socket should be back in freeSockets pool", | ||
| ); | ||
| console.log( | ||
| "Socket back in freeSockets pool:", | ||
| agent.freeSockets[name]?.length, | ||
| ); | ||
|
|
||
| // Third request - should still reuse socket | ||
| const third = await makeRequest("/third"); | ||
| strictEqual(third.reusedSocket, true, "Third request should reuse socket"); | ||
| console.log("Third request: reusedSocket =", third.reusedSocket); | ||
|
|
||
| console.log("All keepAlive tests passed!"); | ||
|
|
||
| server.close(); | ||
| agent.destroy(); | ||
| }); |
There was a problem hiding this comment.
🔵 LOW - HTTP keepAlive spec test lacks error scenario coverage
Agent: testing
Category: quality
Description:
The keepAlive spec test only tests happy path socket reuse with three sequential GET requests. No error handling scenarios (connection errors, timeouts) are covered.
Suggestion:
Extend test coverage to include error scenarios and different HTTP methods (POST, PUT)
Confidence: 60%
Rule: test_new_parameter_coverage
Review ID: 8e3324f7-5f2b-4f78-b80d-469377cc6e2b
Rate it 👍 or 👎 to improve future reviews | Powered by diffray
Review Summary
Validated 78 issues: 32 kept, 46 filtered Issues Found: 32💬 See 14 individual line comment(s) for details. 📊 18 unique issue type(s) across 32 location(s) 📋 Full issue list (click to expand)🟠 HIGH - Floating promise in _writeHeader IIFE (4 occurrences)Agent: nodejs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Incorrect finally handler invocationAgent: nodejs Category: bug File: Description: The code uses .finally(nextTick(onError, this, err, cb)) which calls nextTick() immediately, scheduling the callback right away rather than after the promise settles. nextTick() returns undefined, so .finally() receives undefined instead of a function. Suggestion: Change to .finally(() => nextTick(onError, this, err, cb)) to ensure nextTick is called only after the promise settles. Confidence: 95% Rule: 🟠 HIGH - unwrap() on downcast result can panic (3 occurrences)Agent: rust Category: bug 📍 View all locations
Rule: 🟠 HIGH - Empty catch blocks suppressing errors silently (3 occurrences)Agent: react Category: quality 📍 View all locations
Rule: 🟠 HIGH - Inconsistent optional chaining with direct property access (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟠 HIGH - New property _socketErrorListener lacks test coverage (5 occurrences)Agent: testing Category: quality 📍 View all locations
Rule: 🟠 HIGH - Double type assertion 'as never as Type'Agent: typescript Category: bug File: Description: Using 'as never as net.AddressInfo' is a double assertion that completely bypasses TypeScript's type safety. This pattern is a code smell even if it works. Suggestion: Remove 'as never' and use proper type handling: (server.address() as net.AddressInfo | null)?.port with null check Confidence: 80% Rule: 🟡 MEDIUM - Logic error: always-true condition in _destroyAgent: bugs Category: bug File: Description: Line 1263 sets this.complete = true, then line 1264 checks !this.readableEnded || !this.complete. Since complete is always true at this point, !this.complete is always false. The condition reduces to just !this.readableEnded. Suggestion: Review the logic - if the intent is to check if response was prematurely terminated, the check should probably occur before setting complete=true, or the condition should use &&. Confidence: 75% Rule: 🟡 MEDIUM - JSDoc description slightly misleadingAgent: documentation Category: docs File: Description: Documentation at line 414 says 'Returns the socket' but function actually returns Option. Line 416-417 clarify it returns the resource ID, but the first line is misleading. Suggestion: Update first line to: 'Returns the resource ID for the socket after the HTTP response body has been fully consumed.' Confidence: 75% Rule: 🟡 MEDIUM - Generic error message without context (2 occurrences)Agent: quality Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Using 'any' type for socket propertiesAgent: typescript Category: quality File: Description: Lines 1526-1528 define _socketOverride and socket as 'any | null' with deno-lint-ignore comments. Using 'any' reduces type safety for socket operations. Suggestion: Define proper Socket type or create a discriminated union type for multiple socket types to maintain type safety. Confidence: 65% Rule: 🟡 MEDIUM - Error listener accumulation in IncomingMessageForServerAgent: performance Category: performance File: Description: An error listener is registered on socket via socket?.on('error', ...) in the constructor. For servers handling many requests, if sockets are reused without cleanup, listeners could accumulate. Suggestion: Consider using socket?.once() instead of socket?.on(), or ensure explicit cleanup when the incoming message is destroyed. Confidence: 60% Rule: 🟡 MEDIUM - Unsafe Send/Sync implementations lack comprehensive safety documentationAgent: rust Category: quality File: Description: NodeHttpResourceToBodyAdapter has unsafe Send and Sync implementations with minimal safety documentation. The single-threaded assumption needs more explanation. Suggestion: Add detailed safety documentation explaining the single-threaded executor context and consequences of misuse. Confidence: 72% Rule: 🟡 MEDIUM - Missing documentation for public async operationAgent: rust Category: docs File: Description: Public async function op_node_http_request_with_conn lacks documentation describing its purpose, parameters, and error conditions. Suggestion: Add doc comments explaining the function's purpose, parameters, and what FetchReturn contains. Include information about how keepAlive socket reuse works. Confidence: 75% Rule: 🟡 MEDIUM - HTTP status codes as magic numbers without named constants (2 occurrences)Agent: quality Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Repeated array flattening in _addHeaderLinesAgent: react Category: performance File: Description: Lines 1294 and 1298 both call .flat() on the headers array, creating two separate flattened arrays when one would suffice. Suggestion: Flatten once at the start: Confidence: 75% Rule: 🟡 MEDIUM - Unsafe array access without length checkAgent: typescript Category: bug File: Description: Code checks ArrayIsArray(headers[0]) without first verifying headers has elements. If headers is empty, headers[0] returns undefined which is falsy, making the check fail harmlessly. Suggestion: While not a crash risk, explicit length check improves readability: if (headers.length > 0 && ArrayIsArray(headers[0])) Confidence: 62% Rule: 🔵 LOW - console.error for unimplemented featureAgent: quality Category: style File: Description: Server.setTimeout() uses console.error for a not-implemented feature. This is inconsistent with how other unimplemented features might be handled. Suggestion: Consider throwing an error or using a consistent pattern for not-implemented features across the codebase. Confidence: 65% Rule: ℹ️ 18 issue(s) outside PR diff (click to expand)
🟠 HIGH - Floating promise in _writeHeader IIFE (2 occurrences)Agent: nodejs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Incorrect finally handler invocationAgent: nodejs Category: bug File: Description: The code uses .finally(nextTick(onError, this, err, cb)) which calls nextTick() immediately, scheduling the callback right away rather than after the promise settles. nextTick() returns undefined, so .finally() receives undefined instead of a function. Suggestion: Change to .finally(() => nextTick(onError, this, err, cb)) to ensure nextTick is called only after the promise settles. Confidence: 95% Rule: 🟠 HIGH - Empty catch blocks suppressing errors silently (2 occurrences)Agent: react Category: quality 📍 View all locations
Rule: 🟠 HIGH - Inconsistent optional chaining with direct property access (2 occurrences)Agent: typescript Category: bug 📍 View all locations
Rule: 🟡 MEDIUM - Generic error message without context (2 occurrences)Agent: quality Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Using 'any' type for socket propertiesAgent: typescript Category: quality File: Description: Lines 1526-1528 define _socketOverride and socket as 'any | null' with deno-lint-ignore comments. Using 'any' reduces type safety for socket operations. Suggestion: Define proper Socket type or create a discriminated union type for multiple socket types to maintain type safety. Confidence: 65% Rule: 🟡 MEDIUM - Error listener accumulation in IncomingMessageForServerAgent: performance Category: performance File: Description: An error listener is registered on socket via socket?.on('error', ...) in the constructor. For servers handling many requests, if sockets are reused without cleanup, listeners could accumulate. Suggestion: Consider using socket?.once() instead of socket?.on(), or ensure explicit cleanup when the incoming message is destroyed. Confidence: 60% Rule: 🟡 MEDIUM - Unsafe Send/Sync implementations lack comprehensive safety documentationAgent: rust Category: quality File: Description: NodeHttpResourceToBodyAdapter has unsafe Send and Sync implementations with minimal safety documentation. The single-threaded assumption needs more explanation. Suggestion: Add detailed safety documentation explaining the single-threaded executor context and consequences of misuse. Confidence: 72% Rule: 🟡 MEDIUM - HTTP status codes as magic numbers without named constants (2 occurrences)Agent: quality Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - Repeated array flattening in _addHeaderLinesAgent: react Category: performance File: Description: Lines 1294 and 1298 both call .flat() on the headers array, creating two separate flattened arrays when one would suffice. Suggestion: Flatten once at the start: Confidence: 75% Rule: 🟡 MEDIUM - Unsafe array access without length checkAgent: typescript Category: bug File: Description: Code checks ArrayIsArray(headers[0]) without first verifying headers has elements. If headers is empty, headers[0] returns undefined which is falsy, making the check fail harmlessly. Suggestion: While not a crash risk, explicit length check improves readability: if (headers.length > 0 && ArrayIsArray(headers[0])) Confidence: 62% Rule: 🔵 LOW - console.error for unimplemented featureAgent: quality Category: style File: Description: Server.setTimeout() uses console.error for a not-implemented feature. This is inconsistent with how other unimplemented features might be handled. Suggestion: Consider throwing an error or using a consistent pattern for not-implemented features across the codebase. Confidence: 65% Rule: 🔵 LOW - unwrap() on async stream next() with safety commentAgent: rust Category: bug File: Description: Using unwrap() on stream.as_mut().next().await in library code. The comment explains the safety reasoning but unwrap() in production code can be risky. Suggestion: Consider using expect() with a clearer error message or debug_assert! to validate the assumption. Confidence: 60% Rule: Review ID: |
littledivy
left a comment
There was a problem hiding this comment.
This looks great, nice work!
Closes #29676
Enables HTTP Agent keepAlive connection pooling for improved performance with Node.js HTTP clients.
Changes:
conn.without_shutdown()to allow socket reuse after HTTP responseop_node_http_response_reclaim_connop to return socket to poolConnection: Upgradeheader and usewith_upgrades()for those requestsThis significantly improves performance for HTTP clients using keepAlive, reducing request latency from ~500ms to ~150ms in real-world scenarios like AWS SDK requests.
Results of @littledivy's repro in my M1 Max MacBook Pro:
Node v25.2.1:
Deno 2.6.2 (without these changes):
Deno with these changes: