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

stream: improve view validation on ReadableStreamBYOBRequest.respondWithNewView() #44155

Merged

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Aug 6, 2022

  • This throws if the view is zero-length when there is an active reader when using ReadableStreamBYOBRequest.respondWithNewView().

  • By doing that, we can get all tests passed in wpt/streams/readable-byte-streams/bad-buffers-and-views.any.js.

Refs: https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-with-new-view

  1. If state is "closed"
    5.1. If view.[[ByteLength]] is not 0, throw a TypeError exception.
  2. Otherwise,
    6.1 Assert: state is "readable".
    6.2 If view.[[ByteLength]] is 0, throw a TypeError exception.

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

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 6, 2022
@daeyeon daeyeon changed the title stream: improve views validation on ReadableStreamBYOBRequest stream: improve view validation on ReadableStreamBYOBRequest.respondWithNewView() Aug 6, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2022
@nodejs-github-bot
Copy link
Collaborator

@daeyeon daeyeon added web streams request-ci Add this label to start a Jenkins CI on a PR. labels Aug 7, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2022
@nodejs-github-bot
Copy link
Collaborator

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 7, 2022
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2022
@nodejs-github-bot
Copy link
Collaborator

- This throws if the view is zero-length when there is an active reader
when using `ReadableStreamBYOBRequest.respondWithNewView()`.

- By doing that, we can get all tests passed in
`readable-byte-streams/bad-buffers-and-views.any.js`.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
@daeyeon daeyeon force-pushed the main.webstream-220806.Sat.5609 branch from d2b1fd7 to ee3507e Compare August 11, 2022 01:13
@daeyeon
Copy link
Member Author

daeyeon commented Aug 11, 2022

Rebased since test/wpt/status/streams.json is updated in #43455.

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2022
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 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 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/44155
✔  Done loading data for nodejs/node/pull/44155
----------------------------------- PR info ------------------------------------
Title      stream: improve view validation on `ReadableStreamBYOBRequest.respondWithNewView()` (#44155)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     daeyeon:main.webstream-220806.Sat.5609 -> nodejs:main
Labels     author ready, needs-ci, web streams
Commits    1
 - stream: improve views validation on `BYOBRequest`
Committers 1
 - Daeyeon Jeong 
PR-URL: https://github.com/nodejs/node/pull/44155
Refs: https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-with-new-view
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44155
Refs: https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-with-new-view
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - stream: improve views validation on `BYOBRequest`
   ℹ  This PR was created on Sat, 06 Aug 2022 17:16:39 GMT
   ✔  Approvals: 1
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/44155#pullrequestreview-1064334855
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-08-11T01:31:42Z: https://ci.nodejs.org/job/node-test-pull-request/45991/
- Querying data for job/node-test-pull-request/45991/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2862673144

@lpinca lpinca 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 15, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2022
@nodejs-github-bot nodejs-github-bot merged commit 46f6d22 into nodejs:main Aug 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 46f6d22

@daeyeon daeyeon deleted the main.webstream-220806.Sat.5609 branch August 15, 2022 23:58
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
- This throws if the view is zero-length when there is an active reader
when using `ReadableStreamBYOBRequest.respondWithNewView()`.

- By doing that, we can get all tests passed in
`readable-byte-streams/bad-buffers-and-views.any.js`.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44155
Refs: https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-with-new-view
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
- This throws if the view is zero-length when there is an active reader
when using `ReadableStreamBYOBRequest.respondWithNewView()`.

- By doing that, we can get all tests passed in
`readable-byte-streams/bad-buffers-and-views.any.js`.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: #44155
Refs: https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-with-new-view
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
- This throws if the view is zero-length when there is an active reader
when using `ReadableStreamBYOBRequest.respondWithNewView()`.

- By doing that, we can get all tests passed in
`readable-byte-streams/bad-buffers-and-views.any.js`.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: nodejs#44155
Refs: https://streams.spec.whatwg.org/#readable-byte-stream-controller-respond-with-new-view
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol
Copy link
Member

Depends on #43455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants