fix: premature close with chunked transfer encoding and for async iterators in Node 12 #1172
Conversation
…rators in Node 12 This PR backports the fix from node-fetch#1064 to the `2.x.x` branch following the [comment here](node-fetch#1064 (comment)). I had to add some extra babel config to allow using the `for await..of` syntax in the tests. The config is only needed for the tests as this syntax is not used in the implementation.
Codecov Report
@@ Coverage Diff @@
## 2.x #1172 +/- ##
============================================
- Coverage 100.00% 0 -100.00%
============================================
Files 7 0 -7
Lines 573 0 -573
Branches 183 0 -183
============================================
- Hits 573 0 -573
Continue to review full report at Codecov.
|
|
Ok, I hadn't realised quite how far back in time the supported node versions would go, will see if I can fix up some the oldies. |
src/index.js
Outdated
| properLastChunkReceived = Buffer.compare(buf.slice(-3), LAST_CHUNK) === 0; | ||
| }); | ||
|
|
||
| socket.addListener('close', () => { |
There was a problem hiding this comment.
In #1064 this was socket.prependListener but that's not supported in node 4.
| // Before Node.js 14, pipeline() does not fully support async iterators and does not always | ||
| // properly handle when the socket close/end events are out of order. | ||
| req.on('socket', s => { | ||
| s.addListener('close', hadError => { |
There was a problem hiding this comment.
In #1064 this was s.prependListener but that's not supported in node 4.
| }); | ||
|
|
||
| /* c8 ignore next 18 */ | ||
| if (parseInt(process.version.substring(1)) < 14) { |
There was a problem hiding this comment.
In #1064 this was if (process.version < 'v14') but it fails for node 8.
| req.on('socket', s => { | ||
| s.addListener('close', hadError => { | ||
| // if a data listener is still present we didn't end cleanly | ||
| const hasDataListener = s.listenerCount('data') > 0 |
There was a problem hiding this comment.
#1064 used an undocumented property that held the count of types of events with listeners registered on the socket to determine if the stream was closed prematurely but behaves differently on node 8 and below, so instead if we've got a listener for 'data' events we're still expecting data to arrive so use that to work out if we should emit the ERR_STREAM_PREMATURE_CLOSE error.
|
I had to pin a few deps that have dropped compatibility with old node versions without releasing majors and I've added notes where this PR diverges from the fix in #1064 for the sake of compatibility with old node versions but this should be good for review. |
node-fetch/node-fetch#1064 fixes a bug in node-fetch to make it handle situations where the stream closes prematurely but it's been merged into the v3 release tree which is still future tech with no release date. node-fetch/node-fetch#1172 backports that fix to v2 but although approved it's not been merged and released yet so here we use a temporary fork published with that PR merged in.
| if (headers['transfer-encoding'] === 'chunked' && !headers['content-length']) { | ||
| socket.addListener('close', () => { | ||
| // if a data listener is still present we didn't end cleanly | ||
| const hasDataListener = socket.listenerCount('data') > 0; |
There was a problem hiding this comment.
In #1064 this checks the incoming bytes to make sure it ends with 0\r\n which signifies the last chunk is received. I found this didn't work well against real-world servers as it assumes the 'data' event would align with incoming chunks whereas I don't think there's any such guarantee.
I've reused the same check as the patch for node 10-12 above which seems to work well.
There was a problem hiding this comment.
@achingbrain The data event doesn't have to align; this check is only applicable at the very end when the socket is closed and the last bytes of data are pushed out to the data event. The only way for a chunked transfer to indicate "the end" is for 0\r\n to be the last 3 bytes at this point, which is what the check was for.
There was a problem hiding this comment.
This has caused problems when reusing sockets for multiple requests, as when you use an https.Agent in ‘keep-alive’ mode.
…127) node-fetch/node-fetch#1064 fixes a bug in node-fetch to make it handle situations where the stream closes prematurely but it's been merged into the v3 release tree which is still future tech with no release date. node-fetch/node-fetch#1172 backports that fix to v2 but although approved it's not been merged and released yet so here we use a temporary fork published with that PR merged in.
…ed (#3468) This change fixes #3465 by upgrading to a temporary fork of node-fetch with node-fetch/node-fetch#1172 applied. Co-authored-by: achingbrain <alex@achingbrain.net>
achingbrain
force-pushed
the
fix/premature-connection-close
branch
from
9795205
to
5612867
Compare
|
Hey @xxczaki @bitinn @TimothyGu! Can one of you please have a look an release this? My team is facing huge issues that should be fixed with this patch backported to 2.x.x. Should only take a few minutes of your time! |
…ed (#3468) This change fixes #3465 by upgrading to a temporary fork of node-fetch with node-fetch/node-fetch#1172 applied. Co-authored-by: achingbrain <alex@achingbrain.net>
node-fetch/node-fetch#1172 finally got released so we can remove the fork now.
This PR backports the fix from #1064 to the
2.x.xbranch following the comment here.I had to add some extra babel config to allow using the
for await..ofsyntax in the tests. The config is only needed for the tests as this syntax is not used in the implementation.