WIP: exclusive reader interface

[domenic]

A start at solving #241. /cc @yutakahirano.

Didn't quite get as far as I hoped in terms of spec changes, but got a design doc and reference implementation up. The reference implementation passes all tests except for one, which I will talk about next. There are a lot of tests that need to be written, although the modifications I had to make to existing ones show that this is basically working as desired. The design doc also includes notes on how to optimize.

The test that is failing is the test of using pipeTo on ReadableByteStream. This is failing because we only allow new ExclusiveStreamReader(s) to work if s is a ReadableStream. We do that by checking the existence of the [[reader]] internal slot, which ReadableStreams have but ReadableByteStreams do not.

There are three ways I see of addressing this:

1. Add a slot named [[reader]] to ReadableByteStream, and assume everything "just works." This matches V8 (and possibly other engines), which has "private symbols" it uses and can be shared between classes. However I cannot find any instances of internal slots behaving this way in the ES spec; there is never a case when different classes share the same-named internal slot and algorithms are then meant to apply to both objects the same way. So this is kind of shaky, which leads to:
2. Have two distinct slot names, e.g. [[reader]] and [[byteReader]] or something, and everywhere in the spec that references [[reader]] do some branching logic. Since every single method uses [[reader]] multiple times right now, this would be pretty painful. I think it would basically end up doubling the size of each method. Or we could try to set up some framework ahead of time that allows us to "metaprogram" the slots, e.g. this@[[stream]]@[[readerSlot]] or something. This mostly seems to be spec-level pain though, not implementation or developer-facing pain.
3. Allow arbitrary objects to join into the reader party. This is discussed as "Level 3: custom readable stream implementations?" in the design doc. It would mean switching from new ExclusiveStreamReader(stream) to stream.getReader(), which would be specced as essentially return new ExclusiveStreamReader(this, { getReader: () => this@[[reader]], setReader: v => this@[[reader]] = v }). People creating custom readable stream implementations would then implement their own getReader() method that does e.g. return new ExclusiveStreamReader(stream, { getReader: () => this._reader, setReader: v => this._reader = v }) or uses a weak set or some other mechanism of their choosing. This seems a bit more complicated for implementers, although it's presumably possible to optimize and simplify. E.g. maybe the built-in getReader() doesn't actually have to call new ExclusiveStreamReader(...); it can just set up a version of the class that is implemented with explicit ties to its progenitor. Referencing the design doc, it gives level 1 developers a slightly different interface, and allows level 3 developers to get locking without reimplementing the entire thing themselves.

I will sleep on this a bit. In the meantime thoughts on choosing an approach, as well as code review on the existing stuff, much appreciated.

[domenic]

Implemented option 3 in a follow-up commit. Next up: new tests, then spec.

[domenic]

Added some tests which revealed some issues; see commit message.

I'm starting to feel like I may have dug myself into a hole with option 3 and I want to go back to e.g. option 2.

[yutakahirano]

This function throws when the stream is not locked by this, right?
I think returning a rejected Promise is good, as written in https://github.com/domenic/promises-unwrapping/blob/master/docs/writing-specifications-with-promises.md#promise-returning-functions-should-never-throw.

ditto for .closed and cancel().

[domenic]

Agreed, thanks for catching

[domenic]

I emailed Allen (ES6 spec editor; purposefully not @-mentioning him on GitHub to avoid spamming his email) about whether 1 would be acceptable and he said yes. So, phew, I'm just going to do that.

[domenic]

Right now I am struggling with what to do when the stream closes and/or whether the reader should stop working after the lock is released.

• If we don't automatically release the lock when the stream closes, any consumers of the original stream could be left hanging forever waiting for the stream to close. Maybe this is OK though, because if you forget to release a lock, you're a bad person and you deserve the havoc you've caused?
• If we do auto-release the lock when the stream closes, then we have to be careful. Since the underlying source controls the stream closing, it's outside the control of the user of the reader. So if e.g. state starts throwing upon access when the stream closes, that's going to be bad, unless we force a check for reader.isActive before every reader.state access.
• Maybe we should allow state, ready, and closed---i.e., the non-mutating getters/methods---to be used even on a reader that's been released? That means we can no longer release the reader -> stream pointer when the reader gets released (although we can still release the stream -> reader pointer). That seems OK though.

[domenic]

Tests are looking OK; suggestions for other things to test welcome. I went with not automatically releasing the lock when the stream closes for now, but thoughts welcome. On to the spec work I think.

[domenic]

Skeleton of spec work previewable at https://streams.spec.whatwg.org/branch-snapshots/exclusive-reader/. (Had to fix the deploy process, eek.) A few interesting spots:

• https://streams.spec.whatwg.org/branch-snapshots/exclusive-reader/#locking
• https://streams.spec.whatwg.org/branch-snapshots/exclusive-reader/#rs-get-reader
• https://streams.spec.whatwg.org/branch-snapshots/exclusive-reader/#reader-class

[yutakahirano]

An exclusive stream reader

[domenic]

OK, I updated to a design where closing or erroring the stream auto-releases the lock. Also, when the lock is released, .closed, .ready, and .state still work (by passing through to the stream).

This allows us to make .closed a pass-through, so that it always has the correct semantics. It also is probably nicer for authors to get the auto-release semantics.

I added the TODO'ed tests, and updated the sections of the spec that I'd already written. I'll flesh out the missing parts of the spec next.

[yutakahirano]

lgtm. Thanks!