-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
http: fix keep-alive socket reuse race in requestOnFinish
#61710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
|
The fix can also be backported to Node.js versions 19 and upwards. |
756f872 to
4802f3a
Compare
When the HTTP response ends before the request's 'finish' event fires, responseOnEnd() and requestOnFinish() can both call responseKeepAlive(), corrupting the socket that the agent may have already handed to another request. This commit adds a !req.destroyed guard to requestOnFinish() so the second call is skipped when the socket has already been released. Fixes: nodejs#60001
4802f3a to
52c3851
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61710 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.01%
==========================================
Files 675 675
Lines 204601 204605 +4
Branches 39319 39317 -2
==========================================
- Hits 183611 183610 -1
+ Misses 13275 13270 -5
- Partials 7715 7725 +10
🚀 New features to boost your workflow:
|
pimterry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Nice work on a tricky edge case @martinslota
|
Looks like some checks have failed - some of them related to an event loop delay test (example): I'm not really sure how to fix that. |
When the response ends before the request's
'finish'event fires,responseOnEnd()andrequestOnFinish()can both callresponseKeepAlive(), corrupting the socket that the agent may have already handed to another request.This PR adds a
!req.destroyedguard torequestOnFinish()so the second call is skipped when the socket has already been released.Fixes #60001.
Relates to aws/aws-sdk-js-v3#6426.