Skip to content

stream: fix setting abort reason in ReadableStream.pipeTo() - #44418

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
daeyeon:main.stream-abort-any-220827.Sat.2fc0
Sep 3, 2022
Merged

stream: fix setting abort reason in ReadableStream.pipeTo()#44418
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
daeyeon:main.stream-abort-any-220827.Sat.2fc0

Conversation

@daeyeon

Copy link
Copy Markdown
Member

In 14.2 in the specification, error should be signal’s abort reason. The current behavior seems to assume that only an AbortError instance is given as signal’s abort reason.

  1. If signal is not undefined,
  1. Let abortAlgorithm be the following steps:
  2. Let error be signal’s abort reason. <--

Refs: https://streams.spec.whatwg.org/#readable-stream-pipe-to

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

In 14.2 in the specification, `error` should be signal’s abort reason.
The current behavior seems to assume that only an `AbortError` instance
is given as signal’s abort reason.
Refs: https://streams.spec.whatwg.org/#readable-stream-pipe-to
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Aug 27, 2022
@daeyeon

Copy link
Copy Markdown
MemberAuthor

/cc @nodejs/whatwg-stream

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@daeyeondaeyeon added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 29, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@daeyeondaeyeon added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 3, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 3, 2022
@nodejs-github-bot
nodejs-github-bot merged commit aa90e7a into nodejs:mainSep 3, 2022
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in aa90e7a

@daeyeon
daeyeon deleted the main.stream-abort-any-220827.Sat.2fc0 branch September 3, 2022 14:25
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
In 14.2 in the specification, `error` should be signal’s abort reason.
The current behavior seems to assume that only an `AbortError` instance
is given as signal’s abort reason.
Refs: https://streams.spec.whatwg.org/#readable-stream-pipe-to
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44418
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Sep 5, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
In 14.2 in the specification, `error` should be signal’s abort reason.
The current behavior seems to assume that only an `AbortError` instance
is given as signal’s abort reason.
Refs: https://streams.spec.whatwg.org/#readable-stream-pipe-to
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44418
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
In 14.2 in the specification, `error` should be signal’s abort reason.
The current behavior seems to assume that only an `AbortError` instance
is given as signal’s abort reason.
Refs: https://streams.spec.whatwg.org/#readable-stream-pipe-to
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44418
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
In 14.2 in the specification, `error` should be signal’s abort reason.
The current behavior seems to assume that only an `AbortError` instance
is given as signal’s abort reason.
Refs: https://streams.spec.whatwg.org/#readable-stream-pipe-to
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44418
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.web streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@daeyeon@nodejs-github-bot@mcollina@jasnell@benjamingr