Skip to content

Conversation

@RajeshKumar11
Copy link
Contributor

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

Refs: #48771
Refs: #61658

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Feb 11, 2026
@ronag
Copy link
Member

ronag commented Feb 11, 2026

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

@codecov
Copy link

codecov bot commented Feb 11, 2026

Codecov Report

❌ Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (04946a7) to head (341f0f3).
⚠️ Report is 1 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.73%   -0.01%     
==========================================
  Files         675      675              
  Lines      204648   204655       +7     
  Branches    39330    39327       -3     
==========================================
+ Hits       183651   183657       +6     
- Misses      13282    13285       +3     
+ Partials     7715     7713       -2     
Files with missing lines Coverage Δ
lib/_http_client.js 97.39% <100.00%> (+0.03%) ⬆️
lib/net.js 94.51% <80.00%> (-0.02%) ⬇️

... and 38 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 RajeshKumar11 force-pushed the fix/http-client-socket-error-handler branch from 3f0a317 to cc09fb0 Compare February 11, 2026 08:29
@RajeshKumar11
Copy link
Contributor Author

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.

@RajeshKumar11 RajeshKumar11 force-pushed the fix/http-client-socket-error-handler branch from cc09fb0 to ab1a46b Compare February 11, 2026 09:05
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. tickOnSocket removes and re-adds the listener after the parser
is set up to avoid duplicates. The _destroy path in onSocketNT is also
guarded to prevent double-firing if socketErrorListener already emitted
the error.

Fixes: nodejs#48771
Refs: nodejs#61658
@RajeshKumar11 RajeshKumar11 force-pushed the fix/http-client-socket-error-handler branch from ab1a46b to 341f0f3 Compare February 11, 2026 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants