net: defer synchronous destroy calls in internalConnect

[RajeshKumar11]

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.

Problem

When using http.request() with a custom synchronous lookup function that returns an IP triggering an immediate error (e.g., via blockList), the error was emitted before the HTTP client had set up its error handler. The HTTP client sets up the error handler via process.nextTick in onSocket(), but the connection error occurred synchronously during net.createConnection().

This caused unhandled 'error' events that could not be caught:

const blockList = new net.BlockList();
blockList.addAddress('127.0.0.1');

const req = http.request({
  host: 'example.com',
  port: 80,
  lookup: (hostname, options, cb) => cb(null, '127.0.0.1', 4),
  family: 4,
  createConnection: (opts) => net.createConnection({ ...opts, blockList })
});

req.on('error', (err) => {
  // This was never called - error was unhandled!
});

Solution

Defer socket.destroy() calls to the next tick in:

• internalConnect() - bind errors, blockList errors, and connect errors
• internalConnectMultiple() - timeout and aggregate errors

This gives callers (especially the HTTP client) time to set up error handlers before errors are emitted.

Testing

Added test/parallel/test-http-request-lookup-error-catchable.js that verifies errors are catchable when using http.request with a synchronous custom lookup and blockList.

Fixes: #48771
Refs: #51038

[nodejs-github-bot]

Review requested:

• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.72%. Comparing base (f77a709) to head (f6abca6).
⚠️ Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/net.js	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61658      +/-   ##
==========================================
- Coverage   89.74%   89.72%   -0.03%     
==========================================
  Files         674      674              
  Lines      204360   204368       +8     
  Branches    39265    39270       +5     
==========================================
- Hits       183412   183375      -37     
- Misses      13251    13303      +52     
+ Partials     7697     7690       -7     

Files with missing lines	Coverage Δ
lib/net.js	94.52% <92.30%> (+0.01%)	⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pimterry]

This looks good to me, nice fix, thanks @RajeshKumar11! 👍

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71185/

[jazelly]

nice fix! lgtm

[nodejs-github-bot]

Landed in d8c00ad

[ronag]

This is wrong and is just hiding an issue in the http client. destroy will already defer emitting error.

[pimterry]

@ronag good point, nice catch. Helpfully you've also left a TODO documenting what I think is the real underlying issue inline as well 😃

node/lib/_http_client.js

Lines 948 to 949 in 0777dcc

	// TODO(ronag): Between here and onSocketNT the socket
	// has no 'error' handler.

I think that is exactly what's happening here:

• createConnection successfully returns a socket synchronously to the HTTP client, triggering onSocket. This schedules connection setup including error handling for the next tick (onSocketNT).
• The blocklist failed during that process, but it deferred emitting the error until next tick so handlers can be set up.
• Because the emit-error NT callback was registered first, it fires before the onSocketNT NT callback that would listen for errors, and so there's no error handler registered => 💥

Checking the test behaviour in more detail, that makes sense: in this new test, req.on('error', ...) is actually called before the unhandled error event failure happens. That means request error handler is actually set up, but it's not just actually handling the socket error.

@RajeshKumar11 do you want to take a look and open another PR to improve this fix? The issue is real and this fix does work for the given test, but it would be better to fix in underlying problem directly instead (cleaner, and will fix other bugs too, e.g. any other kinds of next-tick errors like from non-socket streams or similar used in createConnection).

I would guess that we just want to pull the existing request error handler setup to earlier in the process, but it depends what happens in the tests when you do that. At a glance that looks safe but it needs checking and might run into issues, looks like there are some details about how the ordering of agent connections & events need to be handled maybe. Alternatively if there's a very good reason, we could add separate error handling there, just for this case specifically, and then remove it again when onSocketNT fires. Give it a go, feel free to reach out if you run into issues.

[RajeshKumar11]

Thanks for the detailed explanation @pimterry and @ronag!

You're right that this is masking the underlying issue. I'd like to take a shot at fixing the root cause in lib/_http_client.js properly.

From what I understand, the issue is that between onSocket (line 947) and onSocketNT (line 953), the socket has no error handler, so any synchronous/next-tick errors get missed.

I'll investigate moving the error handler setup earlier and open a new PR. Will reach out if I run into issues with the agent connection ordering.

[RajeshKumar11]

I've been working on the proper fix in lib/_http_client.js and I'm running into a timing challenge that I'd like your guidance on.

The Problem:
The error is emitted from the socket before the temporary error handler can be attached. Here's the sequence:

1. http.request() is called with custom createConnection
2. Line 428: opts.createConnection(opts, oncreate) creates socket and error is emitted (via next tick)
3. Line 430: oncreate(null, newSocket) calls onSocket(socket)
4. But by this point, the error has already been queued for emission

I tried attaching a temporary error handler immediately after createConnection returns (in the branch fix/http-client-socket-error-handler), but the test still fails.

Question:
Since the error is emitted via process.nextTick inside net.createConnection (from internalConnect), and http.request also uses process.nextTick for onSocketNT, there's a race between which nextTick callback fires first.

Should the fix be:

1. Attach the error handler synchronously in createConnection path before any ticks happen?
2. Or is there a different approach you had in mind?

I'd appreciate some guidance on the right direction before I go further down this path. Thanks!

[pimterry]

Option 1 I think. Somehow we need to make sure we synchronously attach an error handler after createConnection called, before the next tick.

The main open question is whether we can use the existing error handler (added in tickOnSocket - we could just remove that and add it in advance in onSocket instead) or whether there's some details to how that behaves or how tickOnSocket is used or how the handler works that mean we need a separate temporary error handler that then gets removed next tick.

Hopefully the former since that would be simpler, but you'll need to make the change, test it, and carefully examine & explore the behaviour of the normal socketErrorListener to ensure that there's no unintended behaviour changes or issues with this.

[RajeshKumar11]

Opened a follow-up PR #61770 that fixes the underlying issue properly by attaching socketErrorListener synchronously in onSocket, as suggested by @pimterry.