Possible spec bug with concurrent byob reads

[jasnell]

Take the following test case, where there are multiple concurrent reads on a BYOB reader, where the controller is closed after the first pull, resolving the first read. Unfortunately, it seems the second read never gets resolved. Tested in Node.js and in Chrome.

import { ReadableStream } from 'stream/web';

const rs = new ReadableStream({
  type: 'bytes',
  pull(c) {
    c.byobRequest.respond(10);
    c.close();
  }
});

const reader = rs.getReader({ mode: 'byob' });

console.log("....a");

const results = await Promise.all([
  reader.read(new Uint8Array(10)),
  reader.read(new Uint8Array(10)),
]);

console.log("....b");

When the reads are invoked sequentially and awaited separately it works as expected. When the concurrent reads are both pending, however, closing the controller will never resolve the second read.

/cc @MattiasBuelens

[jasnell]

Looking at ReadableByteStreamControllerClose, there is an existing check to see if pendingPullIntos is not empty, but if it's not, the only check performed there is whether bytesFilled is not zero. When ReadableByteStreamControllerClose defers to ReadableStreamClose, only ReadableStreamDefaultReader pending closes are resolved.

[MattiasBuelens]

The specification expects that you first close the stream, and then commit the BYOB request. Something like:

// respond to first read with some data
c.byobRequest.respond(10);

// respond to second read by closing the stream
c.close();
c.byobRequest.respond(0);

The last respond(0) is necessary because we need to return control of the BYOB view back to the reader.

I agree that this behavior can be a little bit surprising. After all, you didn't even access the second BYOB request, so there's no way you could have modified or detached its view.

Maybe the specification could be a little bit smarter? If controller.[[byobRequest]] is still null when calling ReadableByteStreamControllerClose(), then we definitely know that the BYOB view hasn't been touched yet. So it's safe for us to automatically commit it, and return the view back to the reader.

Keep in mind that, if you do access controller.byobRequest first, you still have to explicitly respond to it:

const byobRequest = c.byobRequest;
c.close();
byobRequest.respond(0); // still required

[domenic]

Eh, having the getter create that kind of side effect seems subpar...

[MattiasBuelens]

Hmm, true. Having controller.[[byobRequest]] populated lazily is more of an optimization, in theory we could also construct it eagerly whenever controller.[[pendingPullIntos]] goes from empty to non-empty. We probably shouldn't attach semantics to it.

In that case: make sure to always respond(0) after close(). 😉

[jasnell]

There's another bug in here that I've found, also relating to the byobRequest getter when autoAllocateChunkSize is used.

const rs = new ReadableStream({
  type: 'bytes',
  autoAllocateChunkSize: 10,
  pull(c) {
    c.enqueue(new Uint8Array(10));
    c.byobRequest.respond(10);
  }
});

const reader = rs.getReader();

console.log(await reader.read());

console.log('test');

This code currently causes the chrome browser tab to crash and Node.js' implementation to assert. The reason is because, while the enqueue() does invalidate the current byobRequest, the pending pull into is not cleared, meaning that the byobRequest is just recreated when the getter is invoked again.

[jasnell]

The behavior is also inconsistent when autoAllocateChunkSize is not used:

This is in edge, but also tested in chrome...

In the first read, the enqueue and subsequent respond appear to work. With the second read, we get an error saying that c.byobRequest is null.

It may very well be that this is just not the way to use the API, which is fine, but the spec and the documentation -- and likely the errors -- should be clearer on what the correct pattern should be.

[jasnell]

The last respond(0) is necessary because we need to return control of the BYOB view back to the reader... I agree that this behavior can be a little bit surprising. After all, you didn't even access the second BYOB request, so there's no way you could have modified or detached its view.

I'd argue that it's more than a little surprising. The assumption here is that the pull algorithm is even aware that there are additional concurrent read requests that it needs to respond to. I'm assuming the only way to do that is to while(c.byobRequest !== null) {}, but as you can see from the second issue here, c.byobRequest is not necessarily guaranteed to be null when using a default reader -- also something the pull algorithm really cannot reliably determine. At the very least this needs to be documented better but I suspect this needs to be addressed in the spec.

Specifically a couple of changes:

1. Calling controller.enqueue(view) should likely remove the pending pull into descriptor as a well as invalidating the current byobRequest
2. Calling controller.close() when there are pending pull intos / byob read requests should automatically resolve those reads and release them back to the reader.
3. The behavior of the controller.byobRequest getter needs to be revisited when autoAllocateChunkSize is used.

[MattiasBuelens]

Keep in mind that BYOB request handling changed a lot in #1123, and Chrome has yet to implement this (Chromium bug #1223565). So you won't get the desired behavior from Chrome. streams/web and web-streams-polyfill should be up-to-date though, so those are valid test cases.

The assumption here is that the pull algorithm is even aware that there are additional concurrent read requests that it needs to respond to. I'm assuming the only way to do that is to while(c.byobRequest !== null) {}

No, you don't need a loop. After you controller.close(), you only need to respond to the current BYOB request. Afterwards, the stream immediately commits all other pull-into descriptors (through ReadableByteStreamControllerRespondInClosedState).

In other words, this will do:

c.close();
c.byobRequest?.respond(0);

I'll have a closer look at your autoAllocateChunkSize case.

[jasnell]

Ok, confirmed that adding byobRequest?.respond(0) after the close works regardless of the number of pending reads. Definitely good but... still not very ergonomic at all. Ideally the close() would handle that for us automatically.

[MattiasBuelens]

Unfortunately, we can't, because the underlying byte source could have transferred the BYOB view's buffer:

const view = c.byobRequest.view;
worker.postMessage(view, [view.buffer]);
c.close();

That's why we have to wait until the source (eventually) calls .respond() or .respondWithNewView(), for example:

worker.onmessage = (event) => {
  c.byobRequest.respondWithNewView(event.message.view);
};

Only then we regain control over the BYOB view, allowing us to resolve the read(view) request(s).

[jasnell]

Ugh, good point. Definitely not ideal!

[MattiasBuelens]

Confirmed, I can get the reference implementation to throw an AssertionError with your test:

promise_test(async () => {
  const rs = new ReadableStream({
    type: 'bytes',
    autoAllocateChunkSize: 10,
    pull(c) {
      c.enqueue(new Uint8Array(10));
      c.byobRequest.respond(10);
    }
  });

  const reader = rs.getReader();

  console.log(await reader.read());
  console.log(await reader.read());

  console.log('test');
}, '#1170');

(I did need to call read() twice though.)

Things start to go wrong immediately in ReadableByteStreamControllerEnqueue. We end up in step 10.2, where we fulfill the read() request but we do not remove the auto-allocated pull-into descriptor. Probably because we assumed that there wouldn't be any that need to be removed, which turns out to be wrong. Woops! 😛

This is definitely a spec bug. Thanks for reporting! 😁

[MattiasBuelens]

We can fix this by doing ReadableByteStreamControllerClearPendingPullIntos in step 10. (We should probably assert that, if there are any pending pull-intos at this step, then there's exactly one and it's an auto-allocated one.)

However, your test case is still invalid. After calling enqueue(), c.byobRequest is invalidated. You only get a new auto-allocated BYOB request after we call pull() again. So with the current code, c.byobRequest will be null and you'll get a "cannot read property 'respond' of null" TypeError instead.

We need a slightly more complicated test case to properly demonstrate the bug:

promise_test(async () => {
  let count = 0;
  const rs = new ReadableStream({
    type: 'bytes',
    autoAllocateChunkSize: 10,
    pull(c) {
      if (count === 0) {
        c.enqueue(new Uint8Array(10));
      } else {
        c.byobRequest.respond(c.byobRequest.view.byteLength);
      }
      count++;
    }
  });

  const reader1 = rs.getReader();
  console.log(await reader1.read());
  reader1.releaseLock();

  const reader2 = rs.getReader({ mode: 'byob' });
  console.log(await reader2.read(new Uint8Array([1, 2, 3])));

  console.log('test');
}, '#1170');

First, we enqueue a chunk to resolve the request from a default reader. The stream will have auto-allocated a BYOB request, but we're not using it. At this point, the stream should remove this pull-into descriptor from its list.

Next, we start a read from a BYOB reader. At this point, this should have become the first pull-into descriptor, and cause the stream to call pull(). But since we never removed the auto-allocated descriptor, we end up in step 11 of ReadableByteStreamControllerPullInto, where we conclude that we don't need to call pull. Woops. 😛

With the proposed fix, this scenario works correctly.

[jasnell]

Tomorrow I'll test out the suggested fix in Node.js and the revised Cloudflare Workers implementation I'm working on now to confirm the approach.

[MattiasBuelens]

1171 has landed with a fix for the AssertionError, so I'm closing this issue. I recommend you consult that PR when updating the Node.js and Cloudflare Workers implementations, because the initially proposed fix didn't cover all of the edge cases.

If you have any questions, feedback or remarks: do let us know! 🙂