Skip to content

Commit 11d5209

Browse files
RajeshKumar11marco-ippolito
authored andcommitted
http: attach error handler to socket synchronously in onSocket
Between onSocket and onSocketNT, the socket had no error handler, meaning any errors emitted during that window (e.g. from a blocklist check or custom lookup) would be unhandled even if the user had set up a request error handler. Fix this by attaching socketErrorListener synchronously in onSocket, setting socket._httpMessage so the listener can forward errors to the request. The _destroy path in onSocketNT is also guarded to prevent double-firing if socketErrorListener already emitted the error. Fixes: #48771 Refs: #61658 PR-URL: #61770 Refs: #48771 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent d36eee1 commit 11d5209

3 files changed

Lines changed: 21 additions & 21 deletions

File tree

lib/_http_client.js

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ function socketErrorListener(err) {
571571
if (req) {
572572
// For Safety. Some additional errors might fire later on
573573
// and we need to make sure we don't double-fire the error event.
574-
req.socket._hadError = true;
574+
socket._hadError = true;
575575
emitErrorEvent(req, err);
576576
}
577577

@@ -910,7 +910,6 @@ function tickOnSocket(req, socket) {
910910
parser.joinDuplicateHeaders = req.joinDuplicateHeaders;
911911

912912
parser.onIncoming = parserOnIncomingClient;
913-
socket.on('error', socketErrorListener);
914913
socket.on('data', socketOnData);
915914
socket.on('end', socketOnEnd);
916915
socket.on('close', socketCloseListener);
@@ -949,8 +948,15 @@ function listenSocketTimeout(req) {
949948
}
950949

951950
ClientRequest.prototype.onSocket = function onSocket(socket, err) {
952-
// TODO(ronag): Between here and onSocketNT the socket
953-
// has no 'error' handler.
951+
// Attach the error listener synchronously so that any errors emitted on
952+
// the socket before onSocketNT runs (e.g. from a blocklist check or other
953+
// next-tick error) are forwarded to the request and can be caught by the
954+
// user's error handler. socketErrorListener requires socket._httpMessage
955+
// to be set so we set it here too.
956+
if (socket && !err) {
957+
socket._httpMessage = this;
958+
socket.on('error', socketErrorListener);
959+
}
954960
process.nextTick(onSocketNT, this, socket, err);
955961
};
956962

@@ -962,8 +968,10 @@ function onSocketNT(req, socket, err) {
962968
if (!req.aborted && !err) {
963969
err = new ConnResetException('socket hang up');
964970
}
965-
// ERR_PROXY_TUNNEL is handled by the proxying logic
966-
if (err && err.code !== 'ERR_PROXY_TUNNEL') {
971+
// ERR_PROXY_TUNNEL is handled by the proxying logic.
972+
// Skip if the error was already emitted by the early socketErrorListener.
973+
if (err && err.code !== 'ERR_PROXY_TUNNEL' &&
974+
!socket?._hadError) {
967975
emitErrorEvent(req, err);
968976
}
969977
req._closed = true;

lib/net.js

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1067,7 +1067,7 @@ function internalConnect(
10671067
err = checkBindError(err, localPort, self._handle);
10681068
if (err) {
10691069
const ex = new ExceptionWithHostPort(err, 'bind', localAddress, localPort);
1070-
process.nextTick(emitErrorAndDestroy, self, ex);
1070+
self.destroy(ex);
10711071
return;
10721072
}
10731073
}
@@ -1077,7 +1077,7 @@ function internalConnect(
10771077

10781078
if (addressType === 6 || addressType === 4) {
10791079
if (self.blockList?.check(address, `ipv${addressType}`)) {
1080-
process.nextTick(emitErrorAndDestroy, self, new ERR_IP_BLOCKED(address));
1080+
self.destroy(new ERR_IP_BLOCKED(address));
10811081
return;
10821082
}
10831083
const req = new TCPConnectWrap();
@@ -1109,20 +1109,12 @@ function internalConnect(
11091109
}
11101110

11111111
const ex = new ExceptionWithHostPort(err, 'connect', address, port, details);
1112-
process.nextTick(emitErrorAndDestroy, self, ex);
1112+
self.destroy(ex);
11131113
} else if ((addressType === 6 || addressType === 4) && hasObserver('net')) {
11141114
startPerf(self, kPerfHooksNetConnectContext, { type: 'net', name: 'connect', detail: { host: address, port } });
11151115
}
11161116
}
11171117

1118-
// Helper function to defer socket destruction to the next tick.
1119-
// This ensures that error handlers have a chance to be set up
1120-
// before the error is emitted, particularly important when using
1121-
// http.request with a custom lookup function.
1122-
function emitErrorAndDestroy(self, err) {
1123-
self.destroy(err);
1124-
}
1125-
11261118

11271119
function internalConnectMultiple(context, canceled) {
11281120
clearTimeout(context[kTimeout]);
@@ -1136,11 +1128,11 @@ function internalConnectMultiple(context, canceled) {
11361128
// All connections have been tried without success, destroy with error
11371129
if (canceled || context.current === context.addresses.length) {
11381130
if (context.errors.length === 0) {
1139-
process.nextTick(emitErrorAndDestroy, self, new ERR_SOCKET_CONNECTION_TIMEOUT());
1131+
self.destroy(new ERR_SOCKET_CONNECTION_TIMEOUT());
11401132
return;
11411133
}
11421134

1143-
process.nextTick(emitErrorAndDestroy, self, new NodeAggregateError(context.errors));
1135+
self.destroy(new NodeAggregateError(context.errors));
11441136
return;
11451137
}
11461138

test/parallel/test-http-request-lookup-error-catchable.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ const net = require('net');
1313
// 2. The lookup returns an IP that triggers a synchronous error (e.g., blockList)
1414
// 3. The error is emitted before http's error handler is set up (via nextTick)
1515
//
16-
// The fix defers socket.destroy() calls in internalConnect to the next tick,
17-
// giving http.request() time to set up its error handlers.
16+
// The fix attaches socketErrorListener synchronously in onSocket so that
17+
// socket errors are forwarded to the request before onSocketNT runs.
1818

1919
const blockList = new net.BlockList();
2020
blockList.addAddress(common.localhostIPv4);

0 commit comments

Comments
 (0)