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

Handle zero-length OK deflate responses (master) #965

Merged
merged 4 commits into from Jan 16, 2022
Merged

Handle zero-length OK deflate responses (master) #965

merged 4 commits into from Jan 16, 2022

Conversation

maciej-irl
Copy link
Contributor

@maciej-irl maciej-irl commented Sep 25, 2020

What is the purpose of this pull request?

  • Bug fix

What changes did you make?

This fixes an issue where older ISS servers (Definitely 7.5, but I can’t rule out other versions), could respond with a zero-length OK deflate response, hanging node-fetch. This was caused by the body stream never emitting a data event. A typical problematic response looks something like this:

HTTP/1.1 200 OK
Content-Encoding: deflate
Content-Length: 0
Server: Microsoft-IIS/7.5
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET

Is there anything you'd like reviewers to know?

This is the master counterpart to the 2.x PR #903
I had to tweak it a bit compared to the 2.x version because pump would incorrectly reject on empty responses.

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Sep 25, 2020

Not really sure how to fix the coverage here. In the original PR adding pump (#670), @xxczaki mentions that the stream error conditions are hard to test.

@uday79936

This comment has been minimized.

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Oct 2, 2020

Don't see what you're saying here, looks like it might be Hacktoberfest spam?

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Oct 21, 2020

Ping @jimmywarting as it’s been a couple weeks. This addresses your comment #903 (comment)

Copy link
Collaborator

@bitinn bitinn left a comment

Sorry for the wait, this PR looks fine to me, but I think a few things can be improved.

  • If the change on pump() error handling is necessary for your tests to pass, then we should probably change all of the instances.

  • I would like to see a comment with link to explain special handling of deflate response, so we have a source of the issues

src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@xxczaki
Copy link
Member

xxczaki commented May 5, 2021

@mgoszcz2 Is it ready for review now? By the way, sorry that you had to wait so long - I wasn't able to participate due to some private issues.

@maciej-irl
Copy link
Contributor Author

maciej-irl commented May 6, 2021

I think so @xxczaki. No bother at all, you can see I was in no rush either.

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jun 26, 2021

Hi, any chance of this getting merged? @xxczaki @bitinn

@tekwiz
Copy link
Member

tekwiz commented Jan 15, 2022

@mgoszcz2 This just needs to be rebased on main. I can approve once that's done, and I believe @bitinn just needs to re-review after that and we can merge.

@tekwiz tekwiz requested a review from bitinn Jan 15, 2022
@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

@tekwiz Thanks. I hope this also means #903 will be merged? Because there was a lot of churn involved in getting to this stage without any merges happening and I don't feel like sinking more time into this pull request. I made #903 to fix a specific issues that was affecting production at a company I no longer work. Unless nothing more is required of me other than a rebase, I'm not willing to continue working on this PR.

@bitinn
Copy link
Collaborator

bitinn commented Jan 16, 2022

bitinn
bitinn approved these changes Jan 16, 2022
@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

Can you explain what's preventing from #903 from being merged? It's a completely different branch and major version. I wasn't even using 3.x, I only made this PR because I was asked to by Jimmy.

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

I rebased it

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

Regarding, #1442: Yes it's the same issue, and in fact that it's a duplicate of #922 I opened in 2020. In case it's not clear, this PR would've fixed it on the main branch, and #903 would've fixed it on the 2.x branch (but neither has been merged).

@bitinn
Copy link
Collaborator

bitinn commented Jan 16, 2022

src/index.js Outdated Show resolved Hide resolved
Copy link

@JefferyHus JefferyHus left a comment

Looks good to me. With only changing err to error

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

Done. Please also consider merging/re-opening #903 when you have some time.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jan 16, 2022

@mgoszcz2

Done. Please also consider merging/re-opening #903 when you have some time.

haven't looked trough (reviewed) it yet... it's closed... what dose that pr do that this pr dose not solve?

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

It fixes this bug for the 2.x branch. You closed it. I do not understand why. When I asked if it will be merged and you said it will be. That was September 2020. I only made this PR because you asked for it to be ported to the main branch too. Please look over your own messages in #903. In retrospect I should have never offered to fix the main branch if I knew merging 6 lines of code in two branches would take nearly two years.

@maciej-irl
Copy link
Contributor Author

maciej-irl commented Jan 16, 2022

I pinged you at the request of David. Please read the messages above.

Thx, for 2.x branch backport we can reopen your PR, please ping Jimmy in the comment. I think a lack of active reviewers at the time was preventing it from moving forward.

@jimmywarting jimmywarting merged commit dd2cea4 into node-fetch:main Jan 16, 2022
8 checks passed
@jimmywarting
Copy link
Collaborator

jimmywarting commented Jan 16, 2022

merged, thx for the fix, again sry that it took so long

@github-actions
Copy link

github-actions bot commented Jan 20, 2022

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Fetch never resolves on empty response with deflate Bug: Zero length deflate responses are never fulfilled
8 participants