Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 08d2e40

Browse filesBrowse files
RajeshKumar11aduh95
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 460f412 commit 08d2e40
Copy full SHA for 08d2e40

3 files changed

+21-21Lines changed: 21 additions & 21 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/_http_client.js‎

Copy file name to clipboardExpand all lines: lib/_http_client.js
+14-6Lines 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;
Collapse file

‎lib/net.js‎

Copy file name to clipboardExpand all lines: lib/net.js
+5-13Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1125,7 +1125,7 @@ function internalConnect(
11251125
err = checkBindError(err, localPort, self._handle);
11261126
if (err) {
11271127
const ex = new ExceptionWithHostPort(err, 'bind', localAddress, localPort);
1128-
process.nextTick(emitErrorAndDestroy, self, ex);
1128+
self.destroy(ex);
11291129
return;
11301130
}
11311131
}
@@ -1135,7 +1135,7 @@ function internalConnect(
11351135

11361136
if (addressType === 6 || addressType === 4) {
11371137
if (self.blockList?.check(address, `ipv${addressType}`)) {
1138-
process.nextTick(emitErrorAndDestroy, self, new ERR_IP_BLOCKED(address));
1138+
self.destroy(new ERR_IP_BLOCKED(address));
11391139
return;
11401140
}
11411141
const req = new TCPConnectWrap();
@@ -1167,20 +1167,12 @@ function internalConnect(
11671167
}
11681168

11691169
const ex = new ExceptionWithHostPort(err, 'connect', address, port, details);
1170-
process.nextTick(emitErrorAndDestroy, self, ex);
1170+
self.destroy(ex);
11711171
} else if ((addressType === 6 || addressType === 4) && hasObserver('net')) {
11721172
startPerf(self, kPerfHooksNetConnectContext, { type: 'net', name: 'connect', detail: { host: address, port } });
11731173
}
11741174
}
11751175

1176-
// Helper function to defer socket destruction to the next tick.
1177-
// This ensures that error handlers have a chance to be set up
1178-
// before the error is emitted, particularly important when using
1179-
// http.request with a custom lookup function.
1180-
function emitErrorAndDestroy(self, err) {
1181-
self.destroy(err);
1182-
}
1183-
11841176

11851177
function internalConnectMultiple(context, canceled) {
11861178
clearTimeout(context[kTimeout]);
@@ -1194,11 +1186,11 @@ function internalConnectMultiple(context, canceled) {
11941186
// All connections have been tried without success, destroy with error
11951187
if (canceled || context.current === context.addresses.length) {
11961188
if (context.errors.length === 0) {
1197-
process.nextTick(emitErrorAndDestroy, self, new ERR_SOCKET_CONNECTION_TIMEOUT());
1189+
self.destroy(new ERR_SOCKET_CONNECTION_TIMEOUT());
11981190
return;
11991191
}
12001192

1201-
process.nextTick(emitErrorAndDestroy, self, new NodeAggregateError(context.errors));
1193+
self.destroy(new NodeAggregateError(context.errors));
12021194
return;
12031195
}
12041196

Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-http-request-lookup-error-catchable.js
+2-2Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.