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

Merged
merged 1 commit into from Jan 16, 2022
Merged

Handle zero-length OK deflate responses #903

merged 1 commit into from Jan 16, 2022

Conversation

nsmaciej
Copy link
Contributor

What is the purpose of this pull request?

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

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?

Our codebase uses node-fetch 2.6.0, but this is still an issue in v3. I’m not sure how to go about patching both branches.

@codecov
Copy link

codecov bot commented Jul 26, 2020

Codecov Report

Merging #903 (0696b20) into 2.x (8c197f8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               2.x      #903   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          579       583    +4     
  Branches       185       186    +1     
=========================================
+ Hits           579       583    +4     
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)

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 8c197f8...0696b20. Read the comment docs.

@jimmywarting
Copy link
Collaborator

@bitinn should we merge this into node-fetch:2.x, master or both?

@nsmaciej
Copy link
Contributor Author

nsmaciej commented Sep 2, 2020

I think merging into 3.x/master would require tweaking the testing parts a bit. I can open another PR if necessary.

@nsmaciej
Copy link
Contributor Author

nsmaciej commented Sep 8, 2020

Ping @bitinn and @jimmywarting, any progress on merging this? I can create another PR for master if necessary

@jimmywarting
Copy link
Collaborator

I can create another PR for master if necessary

Yes please

@nsmaciej
Copy link
Contributor Author

@jimmywarting Ok thanks, will do that soon. This will be merged though, right?

@jimmywarting
Copy link
Collaborator

yes

@nsmaciej
Copy link
Contributor Author

Hi @jimmywarting any chance this can be merged now? It's been nearly two years and I was expecting that this would be merged as soon as I ported my changes to the main branch (which I did in #965), but for whatever reason this didn't happened.

@jimmywarting jimmywarting reopened this Jan 16, 2022
@jimmywarting
Copy link
Collaborator

ty for the fix, sry it took so long. i guess it was never meant for the v2 branch.
this LGMT

@jimmywarting jimmywarting merged commit 838d971 into node-fetch:2.x Jan 16, 2022
@github-actions
Copy link

🎉 This issue has been resolved in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 2.6.8 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants