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: errored stream is disturbed #1134

Merged
merged 19 commits into from Dec 10, 2021
Merged

fix: errored stream is disturbed #1134

merged 19 commits into from Dec 10, 2021

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 8, 2021

No description provided.

@ronag
Copy link
Member Author

ronag commented Dec 8, 2021

@szmarczak This is a spec thing. Care to help clarifying what is the correct behaviour.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2021

Codecov Report

Merging #1134 (2cc7067) into main (b905cea) will decrease coverage by 0.00%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1134      +/-   ##
==========================================
- Coverage   94.30%   94.30%   -0.01%     
==========================================
  Files          40       40              
  Lines        3809     3808       -1     
==========================================
- Hits         3592     3591       -1     
  Misses        217      217              
Impacted Files Coverage Δ
lib/fetch/symbols.js 100.00% <ø> (ø)
lib/fetch/index.js 82.28% <75.00%> (ø)
lib/core/util.js 98.44% <100.00%> (ø)
lib/fetch/body.js 99.16% <100.00%> (-0.01%) ⬇️

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 b905cea...2cc7067. Read the comment docs.

@ronag
Copy link
Member Author

ronag commented Dec 8, 2021

Screenshot 2021-12-08 at 21 03 30

Chrome seems to go with TypeError.

@ronag
Copy link
Member Author

ronag commented Dec 8, 2021

@jimmywarting: Any thoughts? I believe node-fetch fails with an AbortError in this test.

@ronag ronag requested a review from mcollina December 8, 2021 20:10
@jimmywarting
Copy link
Contributor

Umm. is there any wpt test or something in the spec that mention what should actually be happening?

ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021
ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021
ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021
ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021
ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021
@ronag ronag requested a review from mcollina December 9, 2021 09:40
ronag added a commit to nxtedition/node that referenced this pull request Dec 9, 2021
@szmarczak
Copy link
Member

Sorry for my inactivity, had a few tests and an exam in uni. Will take a look now.

lib/core/util.js Outdated Show resolved Hide resolved
test/node-fetch/main.js Show resolved Hide resolved
@szmarczak
Copy link
Member

szmarczak commented Dec 9, 2021

@jimmywarting whatwg/streams#1025 tl;dr unfortunately it's up to Node.js, because they made a design mistake to allow fetch access stream internals.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a unit test?

@ronag
Copy link
Member Author

ronag commented Dec 9, 2021

Can you please add a unit test?

There is already unit test? I updated the relevant one.

@mcollina
Copy link
Member

I missed it. CI is failing.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it should be TypeError 👍

@ronag ronag merged commit a3c0413 into main Dec 10, 2021
ronag added a commit to nodejs/node that referenced this pull request Dec 16, 2021
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Dec 16, 2021
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Dec 17, 2021
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Jan 31, 2022
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Refs: nodejs/undici#1134

PR-URL: nodejs#41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Feb 1, 2022
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* fix: errored stream is disturbed

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* fix: errored stream is disturbed

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup
@Uzlopak Uzlopak deleted the body-error branch February 21, 2024 12:37
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* fix: errored stream is disturbed

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

* fixup

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

Successfully merging this pull request may close these issues.

None yet

5 participants