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

WebStreams "respondWithNewView" incorrectly errors when using autoAllocateChunkSize #41886

Closed
sbquinlan opened this issue Feb 8, 2022 · 2 comments · Fixed by #41887
Closed

Comments

@sbquinlan
Copy link
Contributor

Version

v16.13.2

Platform

Darwin Seans-MacBook-Pro.local 21.2.0 Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:41 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

const stream = new ReadableStream({
  pull: c => {
    const v = new Uint8Array(c.byobRequest.view.buffer, 0, 3);
    v.set([20, 21, 22]);
    c.byobRequest.respondWithNewView(v);
  },
  autoAllocateChunkSize: 10,
  type: 'bytes'
});

const reader = stream.getReader();
const view = reader.read();

How often does it reproduce? Is there a required condition?

Reproduces everytime, it is dependent on using the autoAllocateChunkSize and NOT using a BYOB reader on the stream.

What is the expected behavior?

I expect no error. By the spec (https://streams.spec.whatwg.org/#rs-byob-request-prototype) it should allowed respond with a new view as long as the underlying buffer is the same as the original view. I don't see a mention of this method being specifically only for BYOB readers and not autoAllocated buffers.

Not that this works without error on Chrome (only browser that currently supports byte streams): https://jsfiddle.net/w1jb7rq2/1/

What do you see instead?

RangeError [ERR_INVALID_ARG_VALUE]: The argument 'view' is invalid. Received Uint8Array(3) [ 20, 21, 22 ]
    at new NodeError (node:internal/errors:371:5)
    at readableByteStreamControllerRespondWithNewView (node:internal/webstreams/readablestream:2523:11)
    at ReadableStreamBYOBRequest.respondWithNewView (node:internal/webstreams/readablestream:682:5)
    at Object.pull (/Users/squinlan/Documents/node/test/parallel/test-whatwg-readablebytestream.js:243:21)
    at ensureIsPromise (node:internal/webstreams/util:172:19)
    at readableByteStreamControllerCallPullIfNeeded (node:internal/webstreams/readablestream:2545:5)
    at node:internal/webstreams/readablestream:2662:7 {
  code: 'ERR_INVALID_ARG_VALUE'
}

Additional information

This is due to readableByteStreamControllerPullSteps missing bufferByteLength in the desc added to pendingPullIntos

@benjamingr
Copy link
Member

@MattiasBuelens wdyt?

@MattiasBuelens
Copy link
Contributor

@benjamingr I commented in #41887 (review). Looks like a bug in Node's implementation, which is unfortunately not covered by WPT currently. We should definitely add a test to WPT for this. 👍

nodejs-github-bot pushed a commit that referenced this issue Feb 16, 2022
Fixes: #41886

PR-URL: #41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41886

PR-URL: nodejs#41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41886

PR-URL: nodejs#41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bengl pushed a commit that referenced this issue Feb 21, 2022
Fixes: #41886

PR-URL: #41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bengl pushed a commit that referenced this issue Feb 21, 2022
Fixes: #41886

PR-URL: #41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bengl pushed a commit that referenced this issue Feb 22, 2022
Fixes: #41886

PR-URL: #41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fixes: nodejs#41886

PR-URL: nodejs#41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
Fixes: #41886

PR-URL: #41887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants