http: attach error handler to socket synchronously in onSocket

[RajeshKumar11]

Summary

Fixes the underlying issue identified in #61658 (already merged) where socket errors could be unhandled even when a user had set up a request error handler.

Root Cause

Between onSocket and onSocketNT, the socket had no error handler. If an error was emitted during this window (e.g. from a blocklist check or custom lookup function), it would be unhandled even if the user had req.on('error', ...) set up.

This happened because:

1. createConnection returns the socket synchronously
2. Socket errors are deferred via process.nextTick (in internal/streams/destroy)
3. onSocketNT is also deferred via process.nextTick (to set up socketErrorListener)
4. The error's nextTick fires before onSocketNT's nextTick, so no handler is registered

Fix

Attach socketErrorListener synchronously in onSocket, before the process.nextTick(onSocketNT, ...) call. This requires:

• Setting socket._httpMessage = this early (needed by socketErrorListener to find the request)
• Removing and re-adding the listener in tickOnSocket to avoid duplicates
• Guarding _destroy in onSocketNT against double-firing if socketErrorListener already emitted the error
• Using (req.socket || socket)._hadError since req.socket may not be assigned yet at early error time

Testing

• Added regression test: test/parallel/test-http-request-lookup-error-catchable.js (from net: defer synchronous destroy calls in internalConnect #61658)
• All existing test/parallel/test-http-*.js tests pass

Refs: #48771
Refs: #61658

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[ronag]

Can you revert the other change as part of this PR?

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.76%. Comparing base (04946a7) to head (7d78891).
⚠️ Report is 10 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61770      +/-   ##
==========================================
+ Coverage   89.73%   89.76%   +0.02%     
==========================================
  Files         675      675              
  Lines      204648   204674      +26     
  Branches    39330    39339       +9     
==========================================
+ Hits       183651   183720      +69     
+ Misses      13282    13231      -51     
- Partials     7715     7723       +8     

Files with missing lines	Coverage Δ
lib/_http_client.js	97.38% <100.00%> (+0.01%)	⬆️
lib/net.js	94.51% <80.00%> (-0.02%)	⬇️

... and 49 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.

[RajeshKumar11]

Done! Rebased onto upstream/main and added a revert of the net.js changes from #61658 (commit d8c00ad). The test from #61658 is retained since the underlying issue is now properly fixed in _http_client.js.

[pimterry]

Looks good in general, I think there's just a couple of changes we should make before we merge this to clear things up a bit.

[pimterry]

I think we could simplify this to just

Suggested change

	// Use socket directly as req.socket may not be assigned yet (e.g. when
	// the error is emitted before onSocketNT runs).
	(req.socket || socket)._hadError = true;
	socket._hadError = true;

I've just had a look, and everywhere that socket._httpMessage is set to a request, req.socket is set at the same time, so if req is set they're always equivalent, and setting this on the socket in the socket error handler makes perfect sense to me conceptually.

(It looks like they could diverge if req were null - that logic is less clear - but that doesn't matter within this if)

[pimterry]

I think we can drop both these lines these entirely:

• This method (tickOnSocket) is only called from onSocketNT
• onSocketNT is only called from onSocket
• This socket error handler will always have been set exactly once already, so this doesn't really do anything.

Anything I'm missing? This would make sense if the handlers were different, or maybe if there were other code paths through here, but I don't see any.

[RajeshKumar11]

Thanks @pimterry for the review! Both suggestions applied:

1. Simplified to socket._hadError = true — you are right that req.socket and socket are always equivalent when req is set via socket._httpMessage.

2. Removed the removeListener/on pair from tickOnSocket entirely. Since tickOnSocket is only called from onSocketNT (which is only called from onSocket), and onSocket already attaches the listener once, the duplicate setup was indeed redundant.

Updated the commit message as well to reflect the simpler approach.

[nodejs-github-bot]

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