http: fix just-merged change that could silently truncate responses#64507
http: fix just-merged change that could silently truncate responses#64507pimterry wants to merge 2 commits into
Conversation
Signed-off-by: Tim Perry <pimterry@gmail.com>
|
Review requested:
|
nodejs-github-bot
added
http
|
Fast-track has been requested by @pimterry. Please 👍 to approve. |
|
I'm also ok to revert #64278. |
I think we can roll forwards instead. The issue is real, and the fix is roughly correct, it just misses a couple of cases en route. AFAICT with some small changes this'll work as intended. That said, I say "cases" plural because I've found another one: if the response is completed but the request is still going, this will now swallow errors we previously emitted on the still open request. That can happen because request/response lifetimes are surprisingly independent, servers can finish responding while the request is still going. Most plausible example: the server could send an HTTP 202 to confirm an upload based on headers alone, before the body is fully delivered, expecting the request to keep writing it out even though the response side is closed. It's definitely weird, but it's valid HTTP AFAICT and I wouldn't be surprised at all if somebody does it and expects to get errors if it goes wrong. Bonus fix for that incoming now. |
Signed-off-by: Tim Perry <pimterry@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64507 +/- ##
==========================================
- Coverage 90.23% 90.23% -0.01%
==========================================
Files 741 739 -2
Lines 241692 241668 -24
Branches 45541 45547 +6
==========================================
- Hits 218097 218073 -24
- Misses 15113 15152 +39
+ Partials 8482 8443 -39
🚀 New features to boost your workflow:
|
|
|
||
| if (req) { | ||
| const res = req.res; | ||
| const exchangeComplete = req.writableFinished && res?.complete; |
There was a problem hiding this comment.
I need to metion that, I do these kind of check in my first version of commit ,but it would failed to pass macos tests and it seems that the macos platform has a bit difference.I'm sorry for that I don't deep dive this beacuse I don't have macos device to check the operating system real behavior. I hope this could be a kind of helpful reminder for you. I just think it maybe not easily to fix just check the req status
| assert(response); | ||
| assert.strictEqual(response.complete, true); | ||
| assert.strictEqual(req.writableFinished, false); | ||
| assert.strictEqual(err.code, 'ECONNRESET'); |
There was a problem hiding this comment.
how about replace using this ?
| assert.strictEqual(err.code, 'ECONNRESET'); | |
| assert.strictEqual(err.syscall, 'write'); | |
| switch (err.code) { | |
| case 'ECONNRESET': | |
| case 'ECONNABORTED': | |
| case 'EPIPE': | |
| break; | |
| default: | |
| assert.fail(`Unexpected error code ${err.code}`); | |
| } |
5 participants
This fixes an issue introduced 2 days ago in #64278. With that change, errors that emit after HTTP response headers are received but before the response is completed are swallowed silently.
This PR preserves the core change there, but covers the "response set but not yet completed" window properly so errors here still fire as expected.