Web streams respondWithNewView() errors when view.byteOffset != 0

[Macil]

Version

v18.0.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

If you call ReadableStreamBYOBRequest.respondWithNewView() with a Uint8Array that has a byteOffset greater than 0, then you get an error, even when the regular required conditions are met (the view's byteOffset equals the byobRequest.view's byteOffset, it refers to the same buffer, etc). This makes it impossible to use if the byobRequest.view's byteOffset is greater than 0.

async function main() {
  const readable = new ReadableStream({
    type: "bytes",
    autoAllocateChunkSize: 128,
    pull(controller) {
      const view = controller.byobRequest.view;
      const dest = new Uint8Array(
        view.buffer,
        view.byteOffset,
        view.byteLength
      );
      for (let i = 0; i < dest.length; i++) {
        dest[i] = i;
      }
      controller.byobRequest.respondWithNewView(dest);
      // Code works fine if use .respond() instead of .respondWithNewView():
      // controller.byobRequest.respond(dest.length);
    },
  });

  const reader = readable.getReader({ mode: "byob" });
  
  const buffer = new ArrayBuffer(10);
  const view = new Uint8Array(
    buffer,
    5,
    3
  );
  const read = await reader.read(view);
  console.log("read", read);
  console.log("full buffer", new Uint8Array(read.value.buffer));
}

main();

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

This bug seems to be caused by line 2522 of readablestream.js:

  if (bytesFilled + viewByteOffset > byteLength)
    throw new ERR_INVALID_ARG_VALUE.RangeError('view', view);

byteLength is the length of the view, and bytesFilled is equal to the length of the view in this example (and any other case a read fills up the provided view), so an error would happen whenever the view's byte offset is greater than zero.

I assume byteLength is supposed to be viewBufferByteLength instead here, because it doesn't make sense to compare a calculation based on the view's byte offset to the view's length, but it would make sense to compare that calculation to the underlying buffer's length.

What is the expected behavior?

Here is the expected output that you get when run in Deno or Chrome:

read { value: Uint8Array(3) [ 0, 1, 2 ], done: false }
full buffer Uint8Array(10) [
  0, 0, 0, 0, 0,
  0, 1, 2, 0, 0
]

What do you see instead?

Here is the actual output that you get from Node:

node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^

RangeError [ERR_INVALID_ARG_VALUE]: The argument 'view' is invalid. Received Uint8Array(3) [ 0, 1, 2 ]
    at new NodeError (node:internal/errors:372:5)
    at readableByteStreamControllerRespondWithNewView (node:internal/webstreams/readablestream:2523:11)
    at ReadableStreamBYOBRequest.respondWithNewView (node:internal/webstreams/readablestream:682:5)
    at Object.pull (/Users/macil/Coding/node-webstream-test/test-simple.js:15:30)
    at ensureIsPromise (node:internal/webstreams/util:172:19)
    at readableByteStreamControllerCallPullIfNeeded (node:internal/webstreams/readablestream:2548:5)
    at node:internal/webstreams/readablestream:2666:7 {
  code: 'ERR_INVALID_ARG_VALUE'
}

Node.js v18.0.0

Additional information

I ran into this issue when using code similar to the readInto() example inside the Streams standard for filling a buffer using a bring-your-own-buffer mode reader of a stream that made use of respondWithNewView().

This issue can be worked around in some cases by using respond() instead of respondWithNewView(), but this can only be done when the byobRequest.view has not been transferred, which is easily possible if a ReadableStream is constructed that is wrapping another one.

[austinkelleher]

@Macil I think you're correct...I did a bit of digging on this issue.

I assume byteLength is supposed to be viewBufferByteLength instead here, because it doesn't make sense to compare a calculation based on the view's byte offset to the view's length, but it would make sense to compare that calculation to the underlying buffer's length.

I actually think that the correct value is viewByteLength, which would align more closely with the WHATWG reference implementation: https://github.com/whatwg/streams/blob/033c6d900c438371083785828c659e3150cf4847/reference-implementation/lib/abstract-ops/readable-streams.js#L1345

Further investigation shows that the reference implementation has had some fixes applied 11 months ago: whatwg/streams@033c6d9#diff-db46b8b33e26be52c078fa1540a3516fca7f75af6effb18e9b152efbd114b4bfR1345

It doesn't seem like all of the fixes were applied back to Node.

[austinkelleher]

It also looks like the wpt tests for streams are outdated. I was looking at updating those tests but hit an issue with the update. I'll open a separate issue to track that today.

[saschanaz]

This also causes teed byte stream to throw when being read with non-zero-offset buffer view because of

node/lib/internal/webstreams/readablestream.js

Line 1731 in 167d7a9

readableByteStreamControllerRespondWithNewView(

😬