Skip to content

Commit e70cca7

Browse files
robobunrobobun
authored
node:http: do not give a request the socket of a failed connection (#44191)
### Problem - An `https` request through a proxy emits only `'close'` when the connection ends between the `200` of the CONNECT and the end of the TLS handshake. Node emits `'error'` first: `ECONNRESET`, `Client network socket disconnected before secure TLS connection was established`. - `Agent#createSocket` gives the request the failed socket with the error (`src/js/node/_http_agent.ts:354`, `:530`). The TLS layer set `_hadError` on it, so the guard at `src/js/node/_http_client.ts:1076` skips the `'error'`. ### Fix - `Agent#createSocket` passes only the error, as Node does. `https.Agent#createConnection` closes the proxy connection after every `ERR_PROXY_TUNNEL`, which the request did before. `_http_client.ts` does not change. - Correct: a request gets no socket that it did not listen to. 13 of 13 failure routes equal Node v26.3.0 in events, code and tick (base: 0). - Verified: `test/js/node/http/node-http-proxy-url.test.ts`: 23 tests fail without the fix, 22 also under Node. Also 100 vendored Node tests. - Self-reviewed: 8 concerns raised, 8 addressed. ### Background - The forwarding is from #31587 ([thread](#31587 (comment))), to close a connection that a proxy holds after a refused CONNECT. That thread set aside the close in `createConnection`, which this PR takes. - Considered a guard in the request: one tick after Node, and a custom `createConnection` callback `(err, x)` stays broken. ### Downsides - Not as Node, on purpose: after a refused CONNECT, a direct `https.Agent#createConnection` caller gets a destroyed socket. Node leaves it open. - As in Node: `agent.createSocket()` calls back with `(err)` only. A request with no `'error'` listener throws this `ECONNRESET`. - Not covered: `https-proxy-agent`, `tunnel` and a direct `https` request still report `ERR_SOCKET_CLOSED` (#43381, item 4). <details><summary>Notes</summary> #### Reproduction ```js // bun repro.mjs / node repro.mjs import net from "node:net"; import https from "node:https"; const proxy = net.createServer(s => { s.on("error", () => {}); s.once("data", () => { s.write("HTTP/1.1 200 Connection established\r\n\r\n"); s.once("data", () => s.end()); // the ClientHello }); }); await new Promise(r => proxy.listen(0, "127.0.0.1", r)); const agent = new https.Agent({ proxyEnv: { https_proxy: `http://127.0.0.1:${proxy.address().port}` } }); const req = https.get({ host: "example.invalid", port: 443, path: "/", agent }); req.on("error", e => console.log("error:", e.code, e.message)); req.on("close", () => { console.log("close"); proxy.close(); }); ``` ``` node v26.3.0, and bun with this PR: error: ECONNRESET Client network socket disconnected before secure TLS connection was established close bun 1.4.0, 1.4.1, 1.4.2, main 1313ca6 (3 of 3 runs each, linux-x64): close ``` The proxy ends its side with `end()` after it read the ClientHello. A `destroy()` with unread bytes is a reset, which takes another path through the client and was reported before. The proxy does not have to be at fault: a proxy that relays the close of a target that hangs up during the handshake gives the same result. The report also says that the process never exits. No handle is left open: both sockets are closed and `process.getActiveResourcesInfo()` is empty. The awaited promise never settles, and Bun does not exit on an unsettled top-level await (#33283). #### Who hits it - The built-in proxy support of `node:https`: `new https.Agent({ proxyEnv })`, the global agent under `NODE_USE_ENV_PROXY=1`, and `http.setGlobalProxyFromEnv()`. - An `http://` proxy connection that ends with the `200` or after it, and an `https://` proxy connection that ends during its own TLS handshake or after its `200`. - A request that waited behind `maxSockets`. - No proxy: a custom `Agent#createConnection` that calls back with `(err, socket)`. With a second argument that has no `destroy()` the process ended with `TypeError: socket.destroy is not a function`. The request destroyed a live socket that it was given this way. Node leaves the second argument alone. Not this bug: `https-proxy-agent`, `tunnel` and a direct `https` request, where the request writes to the TLS socket before the handshake is done. They report `ERR_SOCKET_CLOSED` where Node reports `ECONNRESET`, before and after this PR. That is item 4 of #43381, and #43392 is the open PR for it. #### The earlier decision in #31587 The review thread of #31587 found that a proxy which refuses the CONNECT and holds the connection left that connection open, as in Node. It named two ways: forward the socket from the Agent to the request (taken, d677423), or close the connection in `cleanupAndPropagate` for every error (set aside as further from upstream). The forwarding applies to every failed socket creation, not only to a refused CONNECT. So the request also gets the sockets that the TLS layer already marked, and loses its `'error'`. With this PR the Agent is Node's text again, and the one line that differs from Node is the condition in `cleanupAndPropagate`. The test that #31587 added for the held connection passes with no change to its assertions. Node has the same guard since nodejs/node#61770. With an agent whose `createSocket` calls back with `(err, socket)` and a socket that the peer ended during the TLS handshake, Node v26.3.0 also emits only `'close'`. So the forwarding cannot go upstream as it is. Node v26.3.0 leaves the held connection open. #### Measurements Release builds of 1313ca6 and of the three source expressions of this PR on it, linux-x64, and Node v26.3.0. `_http_agent.ts`, `https.ts` and `_http_client.ts` are the same at 1313ca6 and at the base of this branch. Every count is the same in 5 of 5 runs. - builtin JS text: `_http_agent.js` -13 B, `https.js` -33 B, `_http_client.js` 0 B (`wc -c`, `cmp`: identical). bun binary text+data -46 B (`size`: text 80662588 to 80662542, data 110424). - success path: 0 changed bytecode instructions. `onSocketReady` 47, `onSocketCreatedForPending` 45, `cleanupAndPropagateImpl` 36 instructions (base 48/47/38, JSC bytecode dump). - successful request, `nextTick` entries / listener registrations on the socket / `bind` calls, from the moment the Agent has the socket to the `'socket'` event: http 2/9/1, keep-alive 1/5/1, https 2/10/1, tunnel 2/9/0. Base and PR are identical. `createSocket` reported an error 0 of 800 times. - failed creation, `nextTick` entries / listener registrations on the failed socket, from the failure callback to the `'close'` of the request: FIN 1/0 (base 2/1, node 1/0), held 407 1/0 (base 3/1, node 1/1). - 13 of 13 failure routes equal Node v26.3.0 in events, error code and `nextTick` turn (base 0 of 13). FIN routes: `'error'` and `'close'` at turn 1/1 (node 1/1, base no `'error'`, `'close'` at turn 2). The message equals Node's on 11 of 13. The other 2 are texts of the TLS library, the same before and after. - held non-200 CONNECT: the proxy connection is closed for 7 of 7 callers (node 0, base 2) and for 5 of 5 status lines without a code. A direct `createConnection` caller sees `socket.destroyed === true` in its callback (node `false`, base `false`). - custom `createConnection` `cb(err, x)`: 16 of 16 rows equal Node (base 0 of 16), `destroy()` is called on `x` 0 times (node 0, base 8), 0 rows end in an uncaught exception (base 10). The callback of `agent.createSocket()` gets 1 argument after a failure (node 1, base 2). The 13 routes: FIN after the ClientHello, `200` with the FIN, `https://` proxy FIN during its handshake, `https://` proxy `200` then FIN, held 407, 500, RST after the `200`, bytes that are not TLS after the `200`, certificate error, end before the `200`, tunnel timeout, request queued behind `maxSockets`, global agent with `NODE_USE_ENV_PROXY=1`. The 7 callers: a request through the agent, a request queued behind `maxSockets`, `agent.createSocket()`, a subclass that passes on only the error, the `createConnection` option of a request, `agent.createConnection()` with a callback and without one. #### Behavior that changes besides the fix - `'close'` of a request comes one `nextTick` turn earlier after every failed socket creation, on the turn where Node emits it. - The request does not call `destroy()` on what a custom `createConnection` passes with an error. - A subclass that overrides `createSocket` and calls back with `(err, socket)` for a request that waited behind `maxSockets`: the request emits `'error'`, then `'close'`, and does not call `destroy()` on that socket. Before, it emitted only `'close'` when the socket had `_hadError`, and destroyed the socket. The subclass owns that socket now, as in Node. - After a CONNECT reply with a status line that has no usable code (for example `HTTP/1.1 abc`), a direct caller of `createConnection` gets a destroyed socket, as in Node. #### Left as it is - `req.onSocket(socket, err)` called directly with a socket that has `_hadError`, and a subclass that overrides `createSocket` and calls back with `(err, socket)` for a request that did not wait: only `'close'`, also in Node. - The `createConnection` option of a request that reports an error through its callback: `'error'` and no `'close'`, also in Node. - A tunnel that is still pending: while the proxy does not answer the CONNECT, `req.destroy(err)`, `req.abort()`, an aborted signal and `agent.destroy()` emit no `'error'` and no `'close'`, and the proxy connection stays open. When the proxy closes, the request reports `ERR_PROXY_TUNNEL`. Node v26.3.0 does the same. - Two changes of Node's tunnel code after v26.3.0 are not ported here: the limit on the headers of the CONNECT reply (nodejs/node 84e367579e) and the handling of TLS options that `tls.connect()` rejects (nodejs/node#66096). Both are separate work. - Error messages of the TLS library differ from Node's (`WRONG_VERSION_NUMBER`, `self signed certificate`). - `agent.sockets[name]` is deleted after a failed creation. Node keeps an empty array. #### Tests - `test/js/node/http/node-http-proxy-url.node.mts` runs under Node and under Bun. 22 new tests: 4 ways a proxy connection ends, a destroyed and an aborted request, `new http.ClientRequest`, `http.setGlobalProxyFromEnv()`, `diagnostics_channel`, a queued request, 5 tests of a refused CONNECT, 7 of a creation callback with an error and a second argument. 6 of them also assert the `nextTick` turn of `'error'` and `'close'`. - `test/js/node/http/node-http-proxy-url.test.ts`: the global agent with `NODE_USE_ENV_PROXY=1`, in a child process. - linux-x64, debug build. With the fix: 32 of 32 in the dual file (Node v26.3.0: 32 of 32) and 4 of 4 in the wrapper. Without the fix: the 22 and the 1 fail. - Each source edit alone, reverted on a debug build: `_http_agent.ts:350` 17 tests fail, `:524` 1 test fails, `https.ts` 5 tests fail. With `req.onSocket` one `nextTick` late, the 6 turn assertions fail. - CI build 121492 at 6c55f8a ran `node-http-proxy-url.test.ts` and `node-http.test.ts` on every lane, and both pass at the first attempt: Linux glibc and musl, macOS and Windows, each on x64 and arm64, and the ASAN lane. The Node run reports 32 of 32 on each lane. - Windows x64, without the fix: the canary a7c73fd prints only `close` for the reproduction. - Unchanged and passing on linux-x64: `node-http.test.ts` (263 pass), `node-https-agent-checkserveridentity-reuse.test.ts` (30), `node-http-agent-free-socket.test.ts` (9), `node-http-client-request-gc.test.ts`, `trace-events.test.ts`, the two Agent tests of `module-graph-io.test.ts`. - Vendored Node tests: 100 of 101 pass (`test-https-proxy-request-*`, `test-http-proxy-request-*`, `test-http-agent*`, `test-https-agent*`, `test-http-client-abort*`, `test-http-set-global-proxy-from-env-*`, `test-tls-over-http-tunnel`, and more). `test-http-agent-keepalive.js` fails on a debug build with and without this change. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http.test.ts <!-- robobun:evidence:end -->
1 parent 2213a72 commit e70cca7

5 files changed

Lines changed: 581 additions & 17 deletions

File tree

‎src/js/node/_http_agent.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -347,11 +347,7 @@ Agent.prototype.createSocket = function createSocket(req, options, cb) {
347347
// scope so retaining this arrow past its call cannot retain req.
348348
const done = cb;
349349
cb = undefined;
350-
// Pass the socket along with the error: proxy-tunnel failures with a
351-
// statusCode deliberately skip destroy in cleanupAndPropagate so
352-
// req.onSocket can destroy the connection - dropping it here would leak
353-
// a proxy socket that holds its end open after a non-200 CONNECT.
354-
if (err) return done(err, s);
350+
if (err) return done(err);
355351
this.sockets[name] ||= [];
356352
this.sockets[name].push(s);
357353
this.totalSocketCount++;
@@ -525,9 +521,7 @@ function onSocketCreatedForPending(this: any, req, queueName, err, socket) {
525521
}
526522
dropEmptySocketsEntry(this, queueName);
527523
handleSocketAfterProxy(err, req);
528-
// Forward the socket (when the creation error left one behind) so
529-
// onSocketNT can destroy it, like the non-pending path.
530-
req.onSocket(socket ?? null, err);
524+
req.onSocket(null, err);
531525
return;
532526
}
533527

‎src/js/node/https.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -313,9 +313,8 @@ function createConnection(...args) {
313313
// An error occurred during tunnel establishment, in that case just destroy the socket
314314
// and propagate the error to the callback.
315315

316-
// When the error comes from unexpected status code, the stream is still in good shape,
317-
// in that case let req.onSocket handle the destruction instead.
318-
if (err && err.code === "ERR_PROXY_TUNNEL" && err.statusCode === undefined) {
316+
// Node does not destroy after a status code, and nothing else closes that connection: https://github.com/nodejs/node/blob/v26.3.0/lib/https.js#L389-L393
317+
if (err && err.code === "ERR_PROXY_TUNNEL") {
319318
socket.destroy();
320319
}
321320
// This error should go to:

0 commit comments

Comments
 (0)