- From: Domenic Denicola <notifications@github.com>
- Date: Tue, 09 Dec 2014 17:48:59 -0800
- To: whatwg/streams <streams@noreply.github.com>
- Message-ID: <whatwg/streams/pull/251@github.com>
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 it s progen itor. 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. You can merge this Pull Request by running: git pull https://github.com/whatwg/streams exclusive-reader Or you can view, comment on it, or merge it online at: https://github.com/whatwg/streams/pull/251 -- Commit Summary -- * First draft reference implementation of ExclusiveStreamReader * Add a note on how to optimize the delegation -- File Changes -- A Locking Design Doc.md (122) A reference-implementation/lib/exclusive-stream-reader.js (105) M reference-implementation/lib/readable-stream.js (21) M reference-implementation/test/pipe-to.js (46) -- Patch Links -- https://github.com/whatwg/streams/pull/251.patch https://github.com/whatwg/streams/pull/251.diff --- Reply to this email directly or view it on GitHub: https://github.com/whatwg/streams/pull/251
Received on Wednesday, 10 December 2014 01:49:30 UTC