Re: [whatwg/streams] Various fixes for readable byte streams (#1123)

> I apologize for us leaving this PR alone for so long. I had a few weeks in a row of vaccine recovery, vacation, and then working on other stuff. (Although, I see it's still in draft state; is there more to do?)

No worries! Glad to hear you got your vaccinations. The rollout has been a bit slower here in Europe, hopefully I can get one in June or July. 🤞

The draft state is because I wasn't sure if more tests are needed. I'll have another look to see if I can think of other interesting cases that need to be covered.

> Of the goals listed in the OP, I'm slightly concerned about [...] and [...] since they have the potential to break code. I'm not as worried about compat, but slightly worried about potentially shutting down valid use cases.

The way I see it, `.respondWithNewView()` in its current state is *preventing* a lot of valid use cases. These changes are not so much about imposing more restrictions, but actually making it work as advertised.

For example, right now it's *impossible* to use `.respondWithNewView()` when the stream is in the closed state. If you try to call it with a non-empty view:
```javascript
const rs = new ReadableStream({
  type: 'bytes',
  async pull(c) {
    const view = c.byobRequest.view;
    c.close();
    try {
      c.byobRequest.respondWithNewView(view);
    } catch (e) {
      // pull() silences any rejections after calling close(), so we log this separately:
      console.error("respondWithNewView errored with:", e);
    }
  }
});
const reader = rs.getReader({ mode: 'byob' });
await reader.read(new Uint8Array(1));
```
it throws the following error:
> TypeError: Failed to execute 'respondWithNewView' on 'ReadableStreamBYOBRequest': bytes written is not 0

If you change it to `respondWithNewView(view.subarray(0, 0))` to pass a zero-length view (to satisfy the "bytes written must be 0" requirement), then you get this error instead:
> TypeError: Failed to execute 'respondWithNewView' on 'ReadableStreamBYOBRequest': ArrayBufferView is empty

If you're unable to call `respondWithNewView`, then you cannot return ownership of the BYOB request's buffer back to the stream, and so the stream cannot fulfill the pending read-into request. Thus, the stream is stuck.

That's why this change is necessary:
> .respondWithNewView() must now be called with an empty view when the stream is closed. This aligns it with .respond(bytesWritten), which requires bytesWritten to be 0 when closed.

With this, the second case (passing `view.subarray(0, 0)`) would be accepted when the stream is closed. Ownership of the buffer is returned, and the read-into request can be fulfilled.

As for the second change:
> .respondWithNewView(newView) must now be called with a view whose view.buffer.byteLength matches that of the BYOB request. Ideally, we would like to assert that the new view's buffer is the "transferred version" of the BYOB request's buffer, but that's not possible yet with the tools currently provided by the ECMAScript specification.

This is to fulfill the contract of `read(view)`. From the [domintro](https://streams.spec.whatwg.org/#byob-reader-prototype):
> If the chunk does become available, the promise will be fulfilled with an object of the form `{ value: theChunk, done: false }`. In this case, view will be detached and no longer usable, but `theChunk` will be a new view (of the same type) onto the same backing memory region, with the chunk’s data written into it. 

`read(view)` transfers the view's buffer to the stream, which then constructs a BYOB request around it. If we are ever to resolve the `read(view)` promise with a new view *on the same backing memory region*, then the underlying byte source *must give us back the buffer from the BYOB request*.

If it doesn't transfer the BYOB request's view, then it can simply call `.respond()`. The stream will assert that the BYOB request's buffer has not yet been transferred, so it can safely give it back to the result of `read(view)`.

If it *does* transfer the BYOB request's view (e.g. pass it to a worker to fill it), then it must ensure that it (eventually) receives that transferred view back (e.g. the worker passing it back). The stream has no way of knowing where the original BYOB request's view has been transferred to, so the underlying byte source *must* call `.respondWithNewView()`. It's at this point where we would have liked to assert that this new view is indeed backed by the same memory that was in the original BYOB request, so that we can ensure that `read(view)` will get its original buffer back. But alas, there's no way to check this with the current ECMAScript specification, so the closest we can get is to assert that the backing buffer's length is the same as it was when we created the BYOB request (and hope that the underlying byte source didn't do something fishy with it).

In case you want to toy around with transferring views inside `pull()`, here's an example:
```javascript
function transferView(view) {
  // this could also be done with postMessage()
  const rs = new ReadableStream({
    type: 'bytes',
    pull(c) {
      c.byobRequest.respond(c.byobRequest.view.byteLength);
    }
  });
  return rs.getReader({ mode: 'byob' }).read(view).then(result => result.value);
}

const rs = new ReadableStream({
  type: 'bytes',
  async pull(c) {
    var view = await transferView(c.byobRequest.view);
    c.byobRequest.respondWithNewView(view);
  }
});
const reader = rs.getReader({ mode: 'byob' });
await reader.read(new Uint8Array([42])); // should return a result with a transferred but otherwise unmodified view
```

I hope that helps. If it's still unclear, just ask. 😉

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1123#issuecomment-840861165