-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
http: fix http client leaky with double response #60062
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:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60062 +/- ##
=======================================
Coverage 88.45% 88.46%
=======================================
Files 703 703
Lines 207546 207553 +7
Branches 40011 40008 -3
=======================================
+ Hits 183591 183606 +15
+ Misses 15949 15941 -8
Partials 8006 8006
🚀 New features to boost your workflow:
|
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.
One minor point, otherwise LGTM.
or emit a response event then emit an error event on request object ?
Emitting an error here would be a breaking change that will probably break things for some people in real cases. Without that, I don't think this is semver-major, since other than the leak the 2nd response doesn't seem to have been observable until now.
I think emitting an error would probably still be best, because it is an error case where something's clearly gone wrong, and this matches our docs, which say
If any error is encountered during the request (be that with DNS resolution, TCP level errors, or actual HTTP parse errors) an 'error' event is emitted on the returned request object
but that said, it might be better to separate the two issues.
How about we aim to ship this to resolve the leak, and then ship a separate semver-major PR making this into an emitted request error in future?
// Now, parser.incoming is pointed to the new IncomingMessage, | ||
// we need to rewrite it to the first one and skip all the pending IncomingMessage | ||
socket.parser.incoming = req.res; | ||
socket.parser.incoming.skipPendingData = true; |
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.
I think this should probably be a kSkipPendingData
symbol, just to make sure this doesn't accidentally become a public API at some point.
Fixed: #60025
When a request corresponds to multiple responses, ensure that only the first one is processed and skip all subsequent invalid responses (or emit a
response
event then emit anerror
event on request object ?).make -j4 test
(UNIX), orvcbuild test
(Windows) passes