Fix(premature close) Redirect failing when response is chunked but empty

[tekwiz]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• New feature
• Other, please explain:

What changes did you make? (provide an overview)

When an abort signal is given, the test for a properly closed chunked request should be skipped.

Which issue (if any) does this pull request address?

• fix 3.0.0 beta.10: TypeError: Cannot read property 'body' of null #1220
• fix Fix premature close with chunked transfer encoding (Node 10+) and for async iterators in Node 12 #1064
• fix Getting 403 while trying to fetch html data. #1237

Is there anything you'd like reviewers to know?

[jimmywarting]

Going to take a look at this tomorrow.

Just so I know, is #1064 and this PR just a fix to handle a older node:stream that dose things incorrectly?
I did see some if node < x then do this logic...
If we where to drop support for NodeJS version older than 14.13.1 would this still be relevant?

[tekwiz]

@jimmywarting #1064 is actually 2 fixes related to premature close problems. The first part is to handle the bad ending of chunked transfers, which is applicable to to all Node.js 10-14 (and I haven't tested 16). The second part is to fix the async iterator issue with Node.js 12. I will update #1064 to clarify.

[jimmywarting]

The second part is to fix the async iterator issue with Node.js 12

The reason why i asked was that it could be nice to remove the async iterator hack/fix if we decide to remove support for node < 14 sometime later, but i guess NodeJS will have added fetch into core at that point

[tekwiz]

The second part is to fix the async iterator issue with Node.js 12

The reason why i asked was that it could be nice to remove the async iterator hack/fix if we decide to remove support for node < 14 sometime later, but i guess NodeJS will have added fetch into core at that point

Yes, we can definitely remove the async iterator hack/fix next year when Node.js 12 goes EOL (https://github.com/nodejs/Release)

[jimmywarting]

I downloaded your patch and tried both

fetch('https://blog.logrocket.com/feed')
  .then(r => r.arrayBuffer())
  .then(console.log) 
  // works in browser, Throws in node-fetch with Invalid response body while
  // trying to fetch https://fex.baidu.com/feed.xml: Premature close

fetch('https://fex.baidu.com/feed.xml')
  .then(r => r.arrayBuffer())
  .then(console.log) // works in browser, and also this PR

[tekwiz]

@jimmywarting Found the issues. Please give this fix a try.

[jimmywarting]

@jimmywarting Found the issues. Please give this fix a try.

tested, works fine now on nodejs v16.5

I'll do some more testing today.
Originally posted by @tekwiz in #1219 (comment)

Marking as draft then, mark as ready to review when you are done

[jimmywarting]

@tekwiz have you run some tests?

[tekwiz]

@tekwiz have you run some tests?

Yes, apologies for the delay. The day job got in the way. It turns out the Node.js 16 does a better job of handling bad endings of chunked encodings -- it throws an Error with the simple message "aborted" instead of hanging or timing out. However, with the nonspecific-ness of that error and for the sake of consistency, my opinion is to leave the fixResponseChunkedTransferBadEnding check in place for Node.js 16.

[AlenToma]

Tiered of waiting, When will this get checked in and build an npm

[jimmywarting]

Need at least 2 approvals for it to get merged

[LinusU]

Nice 👍

[AlenToma]

Yes at last now, will be greet to check #1237

[jimmywarting]

Thinking we will skip a beta.11 with this pr (fix) and just publish v3.0.0 (stable) instead
The only beef we have tackle now is regarding ESM vs CJS (#1236)
There isn't really any other huge blockers that would bring any more breaking changes to the core (other than the ESM vs CJS discussion we have). The rest can be handled with minor or patch versions.

I thought this PR was needed cuz it broke something that used to work before.

Gona take a closer look at #1237 now