From ea83821413e5467631d7de578eb9512b5e4683a5 Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Sun, 22 Feb 2026 20:17:14 -0800 Subject: [PATCH 1/2] fix: wrap kConnector call in try/catch to prevent client hang When client[kConnector]() throws synchronously (e.g. from tls.connect with bad cert/key), the error was unhandled and left the client in a bad state where subsequent requests would hang indefinitely. Wrapping the call in try/catch with the same cleanup as the error callback (handleConnectError + kResume) restores proper error recovery. Fixes: https://github.com/nodejs/undici/issues/4697 --- lib/dispatcher/client.js | 111 ++++++++++++++++++++------------------- 1 file changed, 58 insertions(+), 53 deletions(-) diff --git a/lib/dispatcher/client.js b/lib/dispatcher/client.js index 101acb60123..671ac199d46 100644 --- a/lib/dispatcher/client.js +++ b/lib/dispatcher/client.js @@ -449,65 +449,70 @@ function connect (client) { }) } - client[kConnector]({ - host, - hostname, - protocol, - port, - servername: client[kServerName], - localAddress: client[kLocalAddress] - }, (err, socket) => { - if (err) { - handleConnectError(client, err, { host, hostname, protocol, port }) - client[kResume]() - return - } + try { + client[kConnector]({ + host, + hostname, + protocol, + port, + servername: client[kServerName], + localAddress: client[kLocalAddress] + }, (err, socket) => { + if (err) { + handleConnectError(client, err, { host, hostname, protocol, port }) + client[kResume]() + return + } - if (client.destroyed) { - util.destroy(socket.on('error', noop), new ClientDestroyedError()) - client[kResume]() - return - } + if (client.destroyed) { + util.destroy(socket.on('error', noop), new ClientDestroyedError()) + client[kResume]() + return + } - assert(socket) + assert(socket) - try { - client[kHTTPContext] = socket.alpnProtocol === 'h2' - ? connectH2(client, socket) - : connectH1(client, socket) - } catch (err) { - socket.destroy().on('error', noop) - handleConnectError(client, err, { host, hostname, protocol, port }) - client[kResume]() - return - } + try { + client[kHTTPContext] = socket.alpnProtocol === 'h2' + ? connectH2(client, socket) + : connectH1(client, socket) + } catch (err) { + socket.destroy().on('error', noop) + handleConnectError(client, err, { host, hostname, protocol, port }) + client[kResume]() + return + } - client[kConnecting] = false - - socket[kCounter] = 0 - socket[kMaxRequests] = client[kMaxRequests] - socket[kClient] = client - socket[kError] = null - - if (channels.connected.hasSubscribers) { - channels.connected.publish({ - connectParams: { - host, - hostname, - protocol, - port, - version: client[kHTTPContext]?.version, - servername: client[kServerName], - localAddress: client[kLocalAddress] - }, - connector: client[kConnector], - socket - }) - } + client[kConnecting] = false + + socket[kCounter] = 0 + socket[kMaxRequests] = client[kMaxRequests] + socket[kClient] = client + socket[kError] = null + + if (channels.connected.hasSubscribers) { + channels.connected.publish({ + connectParams: { + host, + hostname, + protocol, + port, + version: client[kHTTPContext]?.version, + servername: client[kServerName], + localAddress: client[kLocalAddress] + }, + connector: client[kConnector], + socket + }) + } - client.emit('connect', client[kUrl], [client]) + client.emit('connect', client[kUrl], [client]) + client[kResume]() + }) + } catch (err) { + handleConnectError(client, err, { host, hostname, protocol, port }) client[kResume]() - }) + } } function handleConnectError (client, err, { host, hostname, protocol, port }) { From 094160cd0b9dc2d9949dd1560264b8a6be449216 Mon Sep 17 00:00:00 2001 From: Varun Chawla Date: Tue, 17 Mar 2026 21:49:37 -0700 Subject: [PATCH 2/2] test: add tests for synchronous connector throw handling Verify that the client doesn't hang when the connector callback throws synchronously. Tests both the error propagation path and recovery after the connector stops throwing. --- test/node-test/client-connector-throw.js | 85 ++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 test/node-test/client-connector-throw.js diff --git a/test/node-test/client-connector-throw.js b/test/node-test/client-connector-throw.js new file mode 100644 index 00000000000..1556029f124 --- /dev/null +++ b/test/node-test/client-connector-throw.js @@ -0,0 +1,85 @@ +'use strict' + +const { test, after } = require('node:test') +const { tspl } = require('@matteo.collina/tspl') +const { Client } = require('../..') +const { createServer } = require('node:http') + +const { closeServerAsPromise } = require('../utils/node-http') + +test('client does not hang when connector throws synchronously', async (t) => { + const p = tspl(t, { plan: 4 }) + + // We need a server just so the URL resolves, but we'll never actually + // connect because our custom connector throws before that. + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.end() + }) + after(closeServerAsPromise(server)) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`, { + connect: () => { + throw new Error('connector boom') + } + }) + after(() => client.destroy()) + + // First request should get the error from the throwing connector + client.request({ path: '/', method: 'GET' }, (err) => { + p.ok(err instanceof Error) + p.strictEqual(err.message, 'connector boom') + + // Second request should also get an error and not hang + client.request({ path: '/', method: 'GET' }, (err) => { + p.ok(err instanceof Error) + p.strictEqual(err.message, 'connector boom') + }) + }) + }) + + await p.completed +}) + +test('client recovers after connector stops throwing', async (t) => { + const p = tspl(t, { plan: 4 }) + + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.writeHead(200) + res.end('ok') + }) + after(closeServerAsPromise(server)) + + server.listen(0, () => { + let shouldThrow = true + const client = new Client(`http://localhost:${server.address().port}`, { + connect: (opts, cb) => { + if (shouldThrow) { + throw new Error('connector boom') + } + // Fall back to default connector behavior via tls/net + const net = require('node:net') + const socket = net.connect(opts.port, opts.hostname) + socket.on('connect', () => cb(null, socket)) + socket.on('error', (err) => cb(err, null)) + } + }) + after(() => client.destroy()) + + // First request fails because connector throws + client.request({ path: '/', method: 'GET' }, (err) => { + p.ok(err instanceof Error) + p.strictEqual(err.message, 'connector boom') + + // Now stop throwing so the next request can succeed + shouldThrow = false + + client.request({ path: '/', method: 'GET' }, (err, data) => { + p.ifError(err) + p.strictEqual(data.statusCode, 200) + }) + }) + }) + + await p.completed +})