Skip to content

stream: fix ReadableStreamReader.releaseLock()#44292

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
daeyeon:main.220818.Thu.4ea2-.wstream-release-lock
Aug 27, 2022
Merged

stream: fix ReadableStreamReader.releaseLock()#44292
nodejs-github-bot merged 2 commits intonodejs:mainfrom
daeyeon:main.220818.Thu.4ea2-.wstream-release-lock

Conversation

@daeyeon
Copy link
Member

@daeyeon daeyeon commented Aug 19, 2022

This updates ReadableStreamReader.releaseLock() behavior to meet the latest web streams compatibility.

The notable change seems the following:

Attempting to releaseLock() when a reader has pending read requests leads :

  • Old version: releaseLock() will throw a TypeError.
  • Latest version: Not throw a TypeError. Instead, promises returned by reader's ReadableStreamDefaultReader.read() are immediately rejected with a TypeError.

Old version

releaseLock() method steps are:

  1. If ! IsReadableStreamDefaultReader(this) is false, throw a TypeError exception.
  2. If this.[[ownerReadableStream]] is undefined, return.
  3. If this.[[readRequests]] is not empty, throw a TypeError exception. <----
  4. Perform ! ReadableStreamReaderGenericRelease(this).

Latest version

The releaseLock() method steps are:

  1. If this.[[stream]] is undefined, return.
  2. Perform ! ReadableStreamDefaultReaderRelease(this).

Refs: https://streams.spec.whatwg.org/#default-reader-release-lock
Refs: https://streams.spec.whatwg.org/#byob-reader-release-lock
Refs: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/releaseLock

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

@daeyeon
stream: fix ReadableStreamReader.releaseLock()
bbd36fe 
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Aug 19, 2022
@daeyeon
Copy link
Member Author

daeyeon commented Aug 19, 2022

cc @nodejs/whatwg-stream

lpinca
lpinca approved these changes Aug 19, 2022
View reviewed changes
@daeyeon
fixup: remove comments
512e2e7 
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 19, 2022

CI: https://ci.nodejs.org/job/node-test-pull-request/46077/

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2022
mcollina
mcollina approved these changes Aug 22, 2022
View reviewed changes
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

@daeyeon daeyeon added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 26, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 27, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 27, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 27, 2022

Commit Queue failed
- Loading data for nodejs/node/pull/44292
Error: [undefined] GraphQL request Error: Something went wrong while executing your query. Please include `0782:9217:273B87F:285A21E:6309E917` when reporting this issue.
    at Request.query (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/lib/request.js:112:19)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Request.queryAll (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/lib/request.js:136:20)
    at async Request.gql (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/lib/request.js:66:22)
    at async PRData.getReviews (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/lib/pr_data.js:88:20)
    at async Promise.all (index 1)
    at async Promise.all (index 1)
    at async PRData.getAll (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/lib/pr_data.js:58:5)
    at async getMetadata (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/components/metadata.js:18:3)
    at async main (file:///opt/hostedtoolcache/node/16.16.0/x64/lib/node_modules/node-core-utils/components/git/land.js:200:22) {
  data: {
    variables: { after: null, prid: 44292, owner: 'nodejs', repo: 'node' }
  }
}
https://github.com/nodejs/node/actions/runs/2938807443

@daeyeon daeyeon added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit cc75a3c into nodejs:main Aug 27, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 27, 2022

Landed in cc75a3c

@daeyeon daeyeon deleted the main.220818.Thu.4ea2-.wstream-release-lock branch August 27, 2022 12:07
RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
@daeyeon @RafaelGSS
stream: fix ReadableStreamReader.releaseLock()
65134d6 
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44292
Refs: https://streams.spec.whatwg.org/#default-reader-release-lock
Refs: https://streams.spec.whatwg.org/#byob-reader-release-lock
Refs: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/releaseLock
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 5, 2022
Merged
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@daeyeon @Fyko
stream: fix ReadableStreamReader.releaseLock()
b70fce8 
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: nodejs#44292
Refs: https://streams.spec.whatwg.org/#default-reader-release-lock
Refs: https://streams.spec.whatwg.org/#byob-reader-release-lock
Refs: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader/releaseLock
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@juanarbol
Copy link
Member

juanarbol commented Oct 3, 2022

This is breaking wpt tests. Would you mind to backport this to the v16.x release line?

@daeyeon
Copy link
Member Author

daeyeon commented Oct 3, 2022

@juanarbol Thanks for preparing v16.x. Backporting this is pending for now since it depends on both #43455 and #44234.

@MattiasBuelens MattiasBuelens mentioned this pull request Feb 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@lpinca lpinca lpinca approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. web streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@daeyeon @nodejs-github-bot @juanarbol @mcollina @lpinca @aduh95