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

nsmaciej · 2020-09-25T01:01:03Z

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.

nsmaciej · 2020-09-25T01:17:20Z

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.

nsmaciej · 2020-10-02T14:02:57Z

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

nsmaciej · 2020-10-21T11:58:37Z

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

bitinn

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

xxczaki · 2021-05-05T11:52:45Z

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

nsmaciej · 2021-05-06T02:46:39Z

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

nsmaciej · 2021-06-26T21:34:55Z

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

tekwiz · 2022-01-15T22:43:52Z

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

nsmaciej · 2022-01-16T13:01:03Z

@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 · 2022-01-16T13:47:17Z

I believe this is addressing the same issue as #1442 I hope Jeff can help to look into it. We are now 2 years away from my initial review, I am happy to merge but we need to make sure it doesn’t introduce any problems to current codebase.

…

Sent from my iPhone

On Jan 16, 2022, at 21:01, Maciej Goszczycki ***@***.***> wrote: @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. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

nsmaciej · 2022-01-16T15:04:41Z

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.

nsmaciej · 2022-01-16T15:24:43Z

I rebased it

nsmaciej · 2022-01-16T15:30:30Z

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 · 2022-01-16T15:34:45Z

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.

…

Sent from my iPhone

On Jan 16, 2022, at 23:24, Maciej Goszczycki ***@***.***> wrote: I rebased it — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

src/index.js

JefferyHus

Looks good to me. With only changing err to error

nsmaciej · 2022-01-16T22:25:11Z

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

jimmywarting · 2022-01-16T22:56:50Z

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

nsmaciej · 2022-01-16T23:28:00Z

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.

nsmaciej · 2022-01-16T23:37:00Z

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 · 2022-01-16T23:42:00Z

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

github-actions · 2022-01-20T20:43:37Z

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

This comment has been minimized.

Sign in to view

bitinn requested changes Oct 24, 2020

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

src/index.js Show resolved Hide resolved

xxczaki added awaiting-author-response enhancement labels May 5, 2021

LinusU added the needs-rebase label Nov 4, 2021

tekwiz requested a review from bitinn January 15, 2022 22:44

bitinn approved these changes Jan 16, 2022

View reviewed changes

nsmaciej added 3 commits January 16, 2022 16:16

Handle zero-length OK deflate responses

9f3cfc1

Also handle pump callback in the deflate branch

377e09b

Update the use of pump everywhere and link to the original PR

e09bc8d

nsmaciej mentioned this pull request Jan 16, 2022

Handle zero-length OK deflate responses #903

Merged

4 tasks

jimmywarting approved these changes Jan 16, 2022

View reviewed changes

jimmywarting mentioned this pull request Jan 16, 2022

Fetch never resolves on empty response with deflate #1442

Closed

jimmywarting removed enhancement awaiting-author-response needs-rebase labels Jan 16, 2022

jimmywarting reviewed Jan 16, 2022

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

jimmywarting added pending pr awaiting-author-response and removed pending pr labels Jan 16, 2022

JefferyHus approved these changes Jan 16, 2022

View reviewed changes

Address lints

9aa222f

jimmywarting removed the awaiting-author-response label Jan 16, 2022

jimmywarting merged commit dd2cea4 into node-fetch:main Jan 16, 2022

github-actions bot added the released label Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nsmaciej commented Sep 25, 2020 •

edited by jimmywarting

nsmaciej commented Sep 25, 2020

This comment has been minimized.

nsmaciej commented Oct 2, 2020

nsmaciej commented Oct 21, 2020

bitinn left a comment

xxczaki commented May 5, 2021 •

edited

nsmaciej commented May 6, 2021

nsmaciej commented Jun 26, 2021

tekwiz commented Jan 15, 2022

nsmaciej commented Jan 16, 2022

bitinn commented Jan 16, 2022 via email

nsmaciej commented Jan 16, 2022

nsmaciej commented Jan 16, 2022

nsmaciej commented Jan 16, 2022 •

edited

bitinn commented Jan 16, 2022 via email

JefferyHus left a comment

nsmaciej commented Jan 16, 2022

jimmywarting commented Jan 16, 2022

nsmaciej commented Jan 16, 2022

nsmaciej commented Jan 16, 2022

jimmywarting commented Jan 16, 2022

github-actions bot commented Jan 20, 2022

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

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

Conversation

nsmaciej commented Sep 25, 2020 • edited by jimmywarting

nsmaciej commented Sep 25, 2020

This comment has been minimized.

nsmaciej commented Oct 2, 2020

nsmaciej commented Oct 21, 2020

bitinn left a comment

Choose a reason for hiding this comment

xxczaki commented May 5, 2021 • edited

nsmaciej commented May 6, 2021

nsmaciej commented Jun 26, 2021

tekwiz commented Jan 15, 2022

nsmaciej commented Jan 16, 2022

bitinn commented Jan 16, 2022 via email

nsmaciej commented Jan 16, 2022

nsmaciej commented Jan 16, 2022

nsmaciej commented Jan 16, 2022 • edited

bitinn commented Jan 16, 2022 via email

JefferyHus left a comment

Choose a reason for hiding this comment

nsmaciej commented Jan 16, 2022

jimmywarting commented Jan 16, 2022

nsmaciej commented Jan 16, 2022

nsmaciej commented Jan 16, 2022

jimmywarting commented Jan 16, 2022

github-actions bot commented Jan 20, 2022

nsmaciej commented Sep 25, 2020 •

edited by jimmywarting

xxczaki commented May 5, 2021 •

edited

nsmaciej commented Jan 16, 2022 •

edited