Skip to content

Commit c118956

Browse files
RajeshKumar11marco-ippolito
authored andcommitted
net: defer synchronous destroy calls in internalConnect
Defer socket.destroy() calls in internalConnect and internalConnectMultiple to the next tick. This ensures that error handlers have a chance to be set up before errors are emitted, particularly important when using http.request with a custom lookup function that returns synchronously. Previously, if a synchronous lookup function returned an IP that triggered an immediate error (e.g., via blockList), the error would be emitted before the HTTP client had set up its error handler (which happens via process.nextTick in onSocket). This caused unhandled 'error' events. Fixes: #48771 PR-URL: #61658 Refs: #51038 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>
1 parent 50734f8 commit c118956

2 files changed

Lines changed: 60 additions & 5 deletions

File tree

‎lib/net.js‎

Lines changed: 13 additions & 5 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-
self.destroy(ex);
1070+
process.nextTick(emitErrorAndDestroy, self, 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-
self.destroy(new ERR_IP_BLOCKED(address));
1080+
process.nextTick(emitErrorAndDestroy, self, new ERR_IP_BLOCKED(address));
10811081
return;
10821082
}
10831083
const req = new TCPConnectWrap();
@@ -1109,12 +1109,20 @@ function internalConnect(
11091109
}
11101110

11111111
const ex = new ExceptionWithHostPort(err, 'connect', address, port, details);
1112-
self.destroy(ex);
1112+
process.nextTick(emitErrorAndDestroy, self, 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+
11181126

11191127
function internalConnectMultiple(context, canceled) {
11201128
clearTimeout(context[kTimeout]);
@@ -1128,11 +1136,11 @@ function internalConnectMultiple(context, canceled) {
11281136
// All connections have been tried without success, destroy with error
11291137
if (canceled || context.current === context.addresses.length) {
11301138
if (context.errors.length === 0) {
1131-
self.destroy(new ERR_SOCKET_CONNECTION_TIMEOUT());
1139+
process.nextTick(emitErrorAndDestroy, self, new ERR_SOCKET_CONNECTION_TIMEOUT());
11321140
return;
11331141
}
11341142

1135-
self.destroy(new NodeAggregateError(context.errors));
1143+
process.nextTick(emitErrorAndDestroy, self, new NodeAggregateError(context.errors));
11361144
return;
11371145
}
11381146

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
'use strict';
2+
const common = require('../common');
3+
const http = require('http');
4+
const net = require('net');
5+
6+
// This test verifies that errors occurring synchronously during connection
7+
// when using http.request with a custom lookup function and blockList
8+
// can be caught by the error handler.
9+
// Regression test for https://github.com/nodejs/node/issues/48771
10+
11+
// The issue occurs when:
12+
// 1. http.request() is called with a custom synchronous lookup function
13+
// 2. The lookup returns an IP that triggers a synchronous error (e.g., blockList)
14+
// 3. The error is emitted before http's error handler is set up (via nextTick)
15+
//
16+
// The fix defers socket.destroy() calls in internalConnect to the next tick,
17+
// giving http.request() time to set up its error handlers.
18+
19+
const blockList = new net.BlockList();
20+
blockList.addAddress(common.localhostIPv4);
21+
22+
// Synchronous lookup that returns the blocked IP
23+
const lookup = (_hostname, _options, callback) => {
24+
callback(null, common.localhostIPv4, 4);
25+
};
26+
27+
const req = http.request({
28+
host: 'example.com',
29+
port: 80,
30+
lookup,
31+
family: 4, // Force IPv4 to use simple lookup path
32+
createConnection: (opts) => {
33+
// Pass blockList to trigger synchronous ERR_IP_BLOCKED error
34+
return net.createConnection({ ...opts, blockList });
35+
},
36+
}, common.mustNotCall());
37+
38+
// This error handler must be called.
39+
// Without the fix, the error would be emitted before http.request()
40+
// returns, causing an unhandled 'error' event.
41+
req.on('error', common.mustCall((err) => {
42+
if (err.code !== 'ERR_IP_BLOCKED') {
43+
throw new Error(`Expected ERR_IP_BLOCKED but got ${err.code}`);
44+
}
45+
}));
46+
47+
req.end();

0 commit comments

Comments
 (0)