Re: [whatwg/streams] Reject pending reads when releasing reader (#1168)

@MattiasBuelens commented on this pull request.



> +   1. Perform ? [$ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue$](|controller|,
+      |firstPendingPullInto|).

This bit is tricky. `controller.enqueue()` may throw because we failed to allocate a new `ArrayBuffer` to hold any remaining bytes from a now-detached BYOB request:
```javascript
let controller;
const readable = new ReadableStream({
  type: 'bytes',
  start(c) { controller = c }
});
const reader1 = readable.getReader({ mode: 'byob' });

reader1.read(new Uint16Array(1));
// controller.byobRequest.view.byteLength === 2

controller.byobRequest.view[0] = 0xaa;
controller.byobRequest.respond(1);
// not enough to fill the BYOB request, so read() remains pending
// controller.byobRequest.view.byteOffset === 1
// controller.byobRequest.view.byteLength === 1

reader1.releaseLock();
// BYOB request becomes detached from request, but we cannot invalidate it yet
// since the underlying byte source might still be using it

controller.enqueue(new Uint8Array([0xbb]));
// At this point, we must move the bytes from the BYOB request ([0xaa]) into the stream's queue,
// and then push the new chunk ([0xbb]) onto the queue.
// However, moving those bytes means allocating a new ArrayBuffer containing that sub-slice,
// which might fail... and then what do we do? 🤷‍♀️
```
What should the stream do in that case?
* We must not drop any data, so the `[0xaa]` byte has to somehow end up in the queue. We do have ownership over the original `ArrayBuffer`, so it would be nice if we could just "shrink" that buffer somehow without reallocating...
* If we can't prevent data loss, should we error the stream instead? It's kinda weird though.

> @@ -3196,6 +3255,20 @@ The following abstract operations support the implementation of the
     |controller|.[=ReadableByteStreamController/[[queueTotalSize]]=] + |byteLength|.
 </div>
 
+<div algorithm>
+ <dfn abstract-op lt="ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue">ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue(|controller|,

Better name suggestions are welcome. 😛

> +  1. Set [=this=].[=ReadableByteStreamController/[[pendingPullIntos]]=] to the [=list=]
+     « |firstPendingPullInto| ».

Alternatively, we could write:
> Remove all pull-into descriptors after *firstPendingPullInto* from this.\[[pendingPullIntos]].

or:
> Remove all pull-into descriptors whose index is not 0 from this.\[[pendingPullIntos]].

but I don't know if that's a correct usage of [remove](https://infra.spec.whatwg.org/#list-remove). 🙃

> +   1. Perform ? [$ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue$](|controller|,
+      |firstPendingPullInto|).

We could also make `ReadableByteStreamControllerEnqueueDetachedPullIntoToQueue` *not* copy the buffer and instead do:
```javascript
ReadableByteStreamControllerEnqueueChunkToQueue(
  controller,
  firstDescriptor.buffer,
  firstDescriptor.byteOffset,
  firstDescriptor.bytesFilled
);
```
But then there's the risk that when a default reader receives this chunk, we could "leak" data before `byteOffset` or after `byteOffset + bytesFilled` which came from a previous BYOB reader. 😕

Or perhaps we could fix that leak by doing a copy inside `ReadableByteStreamControllerFillReadRequestFromQueue` instead? But then again, *that* could throw an OOM error too... 😖

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/streams/pull/1168#pullrequestreview-844884731

You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/streams/pull/1168/review/844884731@github.com>