http: fix leaked error listener on sync HTTP req create + destroy#62872
Open
pimterry wants to merge 2 commits intonodejs:mainfrom
Open
http: fix leaked error listener on sync HTTP req create + destroy#62872pimterry wants to merge 2 commits intonodejs:mainfrom
pimterry wants to merge 2 commits intonodejs:mainfrom
Conversation
Previously, this resulted in an error listener being added but not cleaned up when the socket was freed back to the agent pool. This also updates the docs to clarify when req.destroy() kills the underlying socket - only if it's called during request processing (more specifically: after onSocketNT, at which point we may have started reading & writing). Signed-off-by: Tim Perry <pimterry@gmail.com>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
http
lpinca
reviewed
Apr 21, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62872 +/- ##
=======================================
Coverage 89.62% 89.63%
=======================================
Files 706 706
Lines 219140 219145 +5
Branches 41984 41981 -3
=======================================
+ Hits 196415 196420 +5
+ Misses 14618 14617 -1
- Partials 8107 8108 +1
🚀 New features to boost your workflow:
|
Signed-off-by: Tim Perry <pimterry@gmail.com>
lpinca
approved these changes
Apr 21, 2026
lpinca
added
request-ci
addaleax
approved these changes
Apr 21, 2026
addaleax
added
the
author ready
github-actions
Bot
removed
the
request-ci
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If you create a request and them immediately destroy it, a socket is created and then freed back to its agent (if there is one) next tick in
onSocketNT, when we discover that the request is already gone before the socket is hooked up.In that past that was fine, but in #61770 error handler setup was made synchronous, rather than waiting for next tick, to handle sync errors during request creation. Error handlers are cleaned up correctly if sockets are normally reused with agents, but not in the specific reuse-at-first-tick flow, because until now it wasn't required.
This change adds cleanup for that specific case. This doesn't leave any gap in error handler coverage - on
'free'the agent either synchronously adds its own error handler here or it assigns the socket to a request (going back to step one: setting up a new request error handler synchronously then initializing inonSocketNT) and we drop the leftover error handler immediately after that returns.This PR also updates the docs to clarify when req.destroy() destroys the underlying socket: after onSocketNT, at which point we've hooked up the socket and may have started reading & writing for real. This matches existing behaviour going back many years, there's no recent change here and I think the behaviour is sane & expected, just making it explicit.