reader.read() inside strategy.size() doesn't work

[ricea]

trafficstars

If I understand correctly, the following test should pass:

promise_test(() => {

  let controller;
  let readPromise;
  let reader;
  const rs = new ReadableStream({
    start(c) {
      controller = c;
    }
  }, {
    size() {
      readPromise = reader.read();
      return 1;
    }
  });
  reader = rs.getReader();
  controller.enqueue();
  return readPromise;

}, 'Readable stream: read() inside strategy size() should resolve');

In practice it times out.

I think the reason is that at step 3 of ReadableStreamDefaultControllerEnqueue it decides that since there isn't a read in process, the chunk should be queued. Then when read() is called inside size() the chunk isn't queued yet, so the read is queued in ReadableStreamAddReadRequest and remains pending.

Probably ReadableStreamDefaultControllerEnqueue should check ReadableStreamGetNumReadRequests a second time.

Or maybe this is too weird to fix.

[ricea]

A related issue that makes me even more uncomfortable is that if you call controller.close() from inside size() with no chunks queued then the stream will be closed but the chunk will be added to the queue, and be stuck there. This violates expectations about the consistency of the internal state. I haven't found a way to make it throw an assertion, mostly because a closed ReadableStream won't do anything at all.

[domenic]

Huh. I share your intuition that this should work. And also that it's code that should never be written.

The full fix would be to do any necessary state checks immediately after calling size(). So I guess after step 4.b in ReadableStreamDefaultControllerEnqueue. Those checks would be... basically steps 2 and 3.

(Also interesting: what if someone releases the reader lock? Seems to work OK.)

I guess at a high level I see three choices:

1. Try to ensure size() is well-behaved. E.g. at one extreme we could have a boolean, [[sizeBeingCalled]], and make everything throw/fail if this flag is true.
2. Allow size() to do whatever it wants, and recover gracefully. So, insert the above checks.
3. Say that if size() is not well-behaved, the behavior will be unexpected (but well-specified). It will essentially be whatever is simplest to spec and implement, i.e. no extra checks.

I think we are currently going for (3). Although you could make an argument that handling exceptions thrown by size() at all, like we do now, is partially leaning toward (2).

I think I am OK with that. It'd be improved a bit if I got around to fixing #427.

We could also probably transition to (1) or (2) later if we really wanted, breaking only pathological code.

[ricea]

We have the semantic that if a read() is in progress then size() isn't called at all. I was initially surprised by this, but it makes sense. However, it means that unlike WritableStream we can't just fix the reentrancy issue by hoisting the call to size() to before any other work is done. Meaning that there is unavoidable cost in overhead for fixing this.

So I'm leaning towards (3), but I do worry that maybe the reason that no assert()s are tripping is because we don't have enough of them.

There is a risk that implementations will track extra internal state that causes them violate their own invariants and crash here. I think I will add tests for the current behaviour so that hopefully such problems can be caught early.

[ricea]

Chromium's implementation of pipeTo() is also affected, as it performs the first read synchronously. See https://github.com/w3c/web-platform-tests/pull/8163. Although the reference implementation and Chromium are observably different here, I don't think either is wrong.

[ricea]

I've written some ReadableStream tests at https://github.com/w3c/web-platform-tests/pull/8166. I'm not sure I've covered every possible case where things can go wrong. As far as I can tell there is no way to call strategySize while already inside a call to strategySize, but I might have missed something.

[ricea]

I think we've decided to live with this rather than change it.

Having said that, I will leave the issue open in case any implementers have problems with it.