Skip to content
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

fix: premature close with chunked transfer encoding and for async iterators in Node 12 #1172

Merged

Conversation

achingbrain
Copy link

This PR backports the fix from #1064 to the 2.x.x branch following the comment here.

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.

…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
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #1172 (9795205) into 2.x (b5e2e41) will decrease coverage by 100.00%.
The diff coverage is n/a.

❗ Current head 9795205 differs from pull request most recent head a39afc0. Consider uploading reports for the commit a39afc0 to get more accurate results
Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
src/body.js
src/headers.js
src/response.js
src/request.js
src/abort-error.js
src/fetch-error.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5417ae...a39afc0. Read the comment docs.

test/test.js Outdated Show resolved Hide resolved
@achingbrain
Copy link
Author

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', () => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

@achingbrain achingbrain May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

@achingbrain
Copy link
Author

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.

test/test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, looks good now!

I do not have access to the npm package myself, so someone else from @node-fetch/core needs to merge and publish the new version

achingbrain added a commit to ipfs/js-ipfs-utils that referenced this pull request May 28, 2021
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;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has caused problems when reusing sockets for multiple requests, as when you use an https.Agent in ‘keep-alive’ mode.

#1767

achingbrain added a commit to ipfs/js-ipfs-utils that referenced this pull request May 28, 2021
…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.
achingbrain added a commit to ipfs/js-ipfs that referenced this pull request Jun 1, 2021
…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 <[email protected]>
@achingbrain achingbrain force-pushed the fix/premature-connection-close branch from 9795205 to 5612867 Compare June 2, 2021 05:55
@oed
Copy link

oed commented Jun 2, 2021

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!

github-actions bot pushed a commit to ipfs-examples/js-ipfs-custom-libp2p that referenced this pull request Jun 14, 2021
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants