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

achingbrain · 2021-05-27T11:02:27Z

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 · 2021-05-27T11:03:53Z

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

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

achingbrain · 2021-05-27T11:47:13Z

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.

achingbrain · 2021-05-27T15:53:31Z

src/index.js

+				properLastChunkReceived = Buffer.compare(buf.slice(-3), LAST_CHUNK) === 0;
+			});
+
+			socket.addListener('close', () => {


In #1064 this was socket.prependListener but that's not supported in node 4.

achingbrain · 2021-05-27T15:53:41Z

src/index.js

+			// 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 => {


In #1064 this was s.prependListener but that's not supported in node 4.

achingbrain · 2021-05-27T15:54:31Z

src/index.js

+		});
+
+		/* c8 ignore next 18 */
+		if (parseInt(process.version.substring(1)) < 14) {


In #1064 this was if (process.version < 'v14') but it fails for node 8.

achingbrain · 2021-05-27T15:56:43Z

src/index.js

+			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


#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 · 2021-05-27T15:58:24Z

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

gr2m

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

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 · 2021-05-28T15:03:47Z

src/index.js

+		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;


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.

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

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

#1767

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

oed · 2021-06-02T20:03:04Z

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>

Uses npm to install deps for examples. We can put yarn back when we remove `node-fetch@2.x` from ipfs-utils, or when yarn can download tarball dependencies reliably. This either needs: 1. node-fetch/node-fetch#1172 merging 2. Swap node-fetch for undici 3. Drop CJS support (node-fetch 3 has the above fix but is ESM-only)

tekwiz · 2022-01-15T21:22:41Z

@jimmywarting I'm good with merging this into 2.x, but I feel it's significant enough to warrant a minor version bump to 2.7.0. I'll leave it to you to merge so you can make the determination on versioning.

aphelionz · 2022-07-16T12:32:54Z

friendly bump :)

achingbrain · 2022-08-11T12:31:05Z

Thanks so much for merging this. I don't think it's been released yet, is there any chance of getting it out of the door?

gr2m · 2022-08-11T20:06:50Z

@jimmywarting we don't have semantic-release setup in the 2.x branch, we need to release manually, or setup semantic-release in there as well. Let me know if you'd like help with the latter

jimmywarting · 2022-08-13T10:04:45Z

@gr2m if you could set it up, then it would be great

…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 · 2022-10-17T07:26:38Z

@gr2m @jimmywarting just a gentle reminder this still hasn't made it into a release. Please could the 2.x.x branch have a release with this in it?

achingbrain · 2023-01-11T11:38:39Z

@gr2m @jimmywarting please, please, please can this be released?

gr2m · 2023-01-13T00:53:54Z

hey sorry I'll look into it

github-actions · 2023-01-13T01:04:46Z

🎉 This PR is included in version 2.6.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

gr2m · 2023-01-13T01:05:25Z

there y'all go. Can you confirm it looks good?

gr2m · 2023-01-13T01:06:48Z

@node-fetch/core I've setup automated releases in the 2.x branch, so backporting should be easy now, just send PRs against the 2.x branch and use the commit conventions

achingbrain · 2023-01-13T12:54:44Z

Thankyouthankyouthankyouthankyou!

node-fetch/node-fetch#1172 finally got released so we can remove the fork now.

achingbrain commented May 27, 2021

View reviewed changes

test/test.js Outdated Show resolved Hide resolved

achingbrain mentioned this pull request May 27, 2021

Fix premature close with chunked transfer encoding (Node 10+) and for async iterators in Node 12 #1064

Merged

4 tasks

achingbrain added 4 commits May 27, 2021 15:49

chore: fix up tests for node 6+

5797fa3

chore: codecov dropped support for node < 8 without shipping major

279c809

chore: npm7 strips empty dependencies hash during install

8ee851f

chore: pin deps to versions that work on node 4

91d4a6c

achingbrain commented May 27, 2021

View reviewed changes

gr2m approved these changes May 27, 2021

View reviewed changes

test/test.js Outdated Show resolved Hide resolved

achingbrain added 2 commits May 27, 2021 21:35

chore: do not emit close error after aborting a request

a614083

chore: test on node 4-16

d127ea1

gr2m approved these changes May 27, 2021

View reviewed changes

achingbrain mentioned this pull request May 28, 2021

fix: use node-fetch fork with fix for stream closed prematurely bug ipfs/js-ipfs-utils#127

Merged

chore: simplify chunked transer encoding bad ending

d821a52

achingbrain commented May 28, 2021

View reviewed changes

chore: avoid calling .destroy as it is not in every node.js release

5612867

achingbrain force-pushed the fix/premature-connection-close branch from 9795205 to 5612867 Compare June 2, 2021 05:55

achingbrain mentioned this pull request Jun 14, 2021

Feat/better fetch ipfs/js-ipfs-utils#126

Closed

chore: listen for response close as socket is reused and shows warnings

2df211d

decentralgabe mentioned this pull request Nov 15, 2021

Node-fetch patch not yet merged upstream ceramicnetwork/.github#161

Closed

achingbrain mentioned this pull request Nov 18, 2021

feat: update dht libp2p/js-libp2p#1009

Merged

3 tasks

achingbrain mentioned this pull request Nov 19, 2021

chore: fix examples libp2p/js-libp2p#1026

Merged

tekwiz approved these changes Jan 15, 2022

View reviewed changes

NotMoni approved these changes Jul 15, 2022

View reviewed changes

jimmywarting merged commit 50536d1 into node-fetch:2.x Jul 16, 2022

achingbrain deleted the fix/premature-connection-close branch July 17, 2022 10:04

achingbrain mentioned this pull request Jan 11, 2023

npm install failed ipfs/js-ipfs-utils#225

Closed

github-actions bot added the released on @2.x label Jan 13, 2023

achingbrain added a commit to ipfs/js-ipfs-utils that referenced this pull request Jan 13, 2023

deps: update node-fetch to 2.6.8

5260a40

node-fetch/node-fetch#1172 finally got released so we can remove the fork now.

achingbrain mentioned this pull request Jan 13, 2023

deps: update node-fetch to 2.6.8 ipfs/js-ipfs-utils#244

Merged

achingbrain added a commit to ipfs/js-ipfs-utils that referenced this pull request Jan 13, 2023

deps: update node-fetch to 2.6.8 (#244)

ebd0f45

node-fetch/node-fetch#1172 finally got released so we can remove the fork now.

roptch mentioned this pull request Jan 16, 2023

Bug with 2.6.8 release throwing exceptions in certain cases #1701

Closed

This was referenced Jun 23, 2023

chore(deps): Move node-fetch to ^2.6.9 googleapis/teeny-request#308

Merged

chore(deps): Upgrade node-fetch to ^2.6.9 googleapis/gaxios#541

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achingbrain commented May 27, 2021

codecov bot commented May 27, 2021 •

edited

achingbrain commented May 27, 2021

achingbrain May 27, 2021

achingbrain May 27, 2021

achingbrain May 27, 2021

achingbrain May 27, 2021 •

edited

achingbrain commented May 27, 2021

gr2m left a comment

achingbrain May 28, 2021

tekwiz Jul 27, 2021

steveluscher Aug 3, 2023

oed commented Jun 2, 2021

tekwiz commented Jan 15, 2022

aphelionz commented Jul 16, 2022

achingbrain commented Aug 11, 2022

gr2m commented Aug 11, 2022

jimmywarting commented Aug 13, 2022

achingbrain commented Oct 17, 2022

achingbrain commented Jan 11, 2023

gr2m commented Jan 13, 2023

github-actions bot commented Jan 13, 2023

gr2m commented Jan 13, 2023

gr2m commented Jan 13, 2023

achingbrain commented Jan 13, 2023

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

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

Conversation

achingbrain commented May 27, 2021

codecov bot commented May 27, 2021 • edited

Codecov Report

achingbrain commented May 27, 2021

achingbrain May 27, 2021

Choose a reason for hiding this comment

achingbrain May 27, 2021

Choose a reason for hiding this comment

achingbrain May 27, 2021

Choose a reason for hiding this comment

achingbrain May 27, 2021 • edited

Choose a reason for hiding this comment

achingbrain commented May 27, 2021

gr2m left a comment

Choose a reason for hiding this comment

achingbrain May 28, 2021

Choose a reason for hiding this comment

tekwiz Jul 27, 2021

Choose a reason for hiding this comment

steveluscher Aug 3, 2023

Choose a reason for hiding this comment

oed commented Jun 2, 2021

tekwiz commented Jan 15, 2022

aphelionz commented Jul 16, 2022

achingbrain commented Aug 11, 2022

gr2m commented Aug 11, 2022

jimmywarting commented Aug 13, 2022

achingbrain commented Oct 17, 2022

achingbrain commented Jan 11, 2023

gr2m commented Jan 13, 2023

github-actions bot commented Jan 13, 2023

gr2m commented Jan 13, 2023

gr2m commented Jan 13, 2023

achingbrain commented Jan 13, 2023

codecov bot commented May 27, 2021 •

edited

achingbrain May 27, 2021 •

edited