Make .ready fulfill-only?

[domenic]

Continuing from #243 (comment):

The essential issue is that right now rs.ready.then(f, r) might call f at a time when the stream is actually in an errored state, if the underlying source or sink causes an error between becoming readable and f being called. So the story is something like "usually if f is called it'll be readable or closed, and whenever it's errored it'll call r with the error ... except not always."

A few issues, none of them too serious:

Slightly more awkward for readable streams

I tried to change this and noticed it made one example a bit more awkward:

function streamToConsole(readableStream) {
  pump();

  function pump() {
    while (readableStream.state === "readable") {
      console.log(readableStream.read());
    }

    if (readableStream.state === "closed") {
      console.log("--- all done!");
    } else {
      // If we're in an error state, the returned promise will be rejected with
      // that error, so no need to handle "waiting" vs. "errored" separately.
      readableStream.ready.then(pump, e => console.error(e));
    }
  }
}

becomes something like

function streamToConsole(readableStream) {
  readableStream.closed.then(
    () => console.log("--- all done!"),
    e => console.error(e);
  );

  pump();

  function pump() {
    while (readableStream.state === "readable") {
      console.log(readableStream.read());
    }

    if (readableStream.state === "waiting") {
      readableStream.ready.then(pump);
    }

    // otherwise "closed" or "errored", which will be handled above.
  }
}

That is, it becomes mandatory to interact with the .closed promise now if you want access to the error object. Note that this doesn't work:

function streamToConsole(readableStream) {
  pump();

  function pump() {
    while (readableStream.state === "readable") {
      console.log(readableStream.read());
    }

    if (readableStream.state === "closed") {
      console.log("--- all done!");
    } else if (readableStream.state === "errored") {
      // uh oh how do I get the error object? :(
    } else {
      // must be waiting
      readableStream.ready.then(pump);
    }
  }
}

Also note that the first version (i.e. with the current spec) isn't actually buggy, since if pump is called when the stream is errored it will just call .ready.then(pump, logTheError) again.

What do we do for writable streams?

Writable streams have basically the same problem. However we reject their .ready promise more often, i.e. when the stream begins closing, or is closed. Do we also want to eliminate those cases? My gut instinct is that yes, we do. In fact the current setup is not great.

---

So what do you guys think, @tyoshino and @yutakahirano? Can we live with the change in consumer style? And are we OK making it fulfill-only for writable streams too? I think both are OK but wanted to discuss first.

[tyoshino]

mandatory to interact with the .closed promise

Right. We can also do that at // uh oh how do I get the error object? :(. Current ReadableStream.prototype.pipeTo() does that in doPipe().

   dest.ready.catch(cancelSource);

How does this method look to you?

An extra microtask is required to retrieve the exception. We can expose storedError for synchronous read but it seems overkilling.

[domenic]

We can also do that at // uh oh how do I get the error object? :(

Yeah I guess it delays a microtask and the code looks a bit strange, but it's not too big of a deal.

Current ReadableStream.prototype.pipeTo() does that in doPipe().

I was able to modify it locally to just use .closed instead of .ready in most cases, and it worked fine (all tests pass).

We can expose storedError for synchronous read but it seems overkilling.

I agree.

[tyoshino]

Re: WritableStream, I agree with you. It can also be fulfill-only. .closed can give us storedError.

i.e. when the stream begins closing, or is closed.

Good point. .ready is rejected on transition to "closing" and again (replaced with a rejected promise) on transition to "errored". If we make this fulfill-only, we cannot distinguish between them using .ready (with the current behavior, they're rejected with different errors). Maybe that's the only change of capability, and is not so important, I think.

We can say the same about ReadableStream's .ready. With the current behavior, we can know that the readable stream became readable once before we check state in the fulfillment callback to see it being "errored". I'd say that this is also not so important functionality.