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 (Node 10+) and for async iterators in Node 12 #1064

Merged

Conversation

tekwiz
Copy link
Member

@tekwiz tekwiz commented Jan 10, 2021

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)

re. 47b00e1: For responses using chunked Transfer-Encoding without a Content-Length, this adds a check at the socket's close event to verify that the last bytes received were the final chunk bytes, i.e. 0\r\n. If not, it creates a Premature close error and sends it to response.body.destroy(). Certain network errors evidently bypass the HTTP parser's error handling resulting in silent failures. @davidje13 confirmed fix in #739 #739 (comment).

re. 13cdcf3: 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. This fix emits a Premature close error if the socket's readable end event occurs before the writable close event.

re. 9cf8c18: Usage of async iterators for both Node.js 12 and 14 is added to the README.

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

#739 #766 #968 #1055 #1056 and possibly #736

Is there anything you'd like reviewers to know?

@tekwiz tekwiz force-pushed the fix/chunked-transfer-premature-close2 branch from 9cf8c18 to 858b7ed Compare January 26, 2021 01:11
@oed
Copy link

oed commented Jan 26, 2021

Very excited for this fix! 😄

@tekwiz
Copy link
Member Author

tekwiz commented Jan 28, 2021

@davidje13 @xxczaki @Richienb I'm seeing anecdotal evidence from multiple sources that this change fixes a large set of issues with stalling requests. I think it would be beneficial to get your reviews so we can merge and release an updated beta.

@oed
Copy link

oed commented Apr 29, 2021

Hey @tekwiz is there a plan to release this bugfix in a new 2.x.x release? Still waiting for a resolution of the issue that this fixes!

@ran-kalir
Copy link

ran-kalir commented May 4, 2021

are these changes released to 3.x.x? i see the changes in master but when i run npm i node-fetch@next i cant find them in the node_modules.

@xxczaki
Copy link
Member

xxczaki commented May 4, 2021

@ran-kalir @oed I will release 3.0.0-beta.10 once #1141 is merged 😄

@xxczaki
Copy link
Member

xxczaki commented May 4, 2021

(...) is there a plan to release this bugfix in a new 2.x.x release? Still waiting for a resolution of the issue that this fixes!

cc @bitinn - what do you think? sorry for the ping

@oed No, because this PR was made in the v3 branch.

@oed
Copy link

oed commented May 5, 2021

Ok, that's unfortunate. When is v3 slated for release? Or any way to backport this fix?

@xxczaki
Copy link
Member

xxczaki commented May 5, 2021

@oed The next beta version (3.0.0-beta.10) will be released once #1141 is merged. When it comes to the stable v3 release, it should happen later this year. I'm not sure about backporting this fix and releasing yet another v2 release - would like to hear thoughts from @node-fetch/core.

@stbrody
Copy link

stbrody commented May 26, 2021

Hey @xxczaki, any update on getting this backported to v2? This is causing major problems for us that is getting in the way of our product launch. (see #1055 and ipfs/js-ipfs#3465 which inspired it)

CC @oed @Gozala

@gr2m
Copy link
Collaborator

gr2m commented May 26, 2021

@stbrody can you send a pull request agains the v2 branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet