Should we "lock" readable streams while piping?

[domenic]

Inspired by:

• Backpressure on fetch integrated with Streams w3c/ServiceWorker#452 (comment)
• Ensure the JS thread doesn't have to get hit when piping two streams to each other #97
• An offline email conversation with @acolwell

Right now, if you are piping a readable stream to a writable stream, you can also concurrently call .read(). Usually this will just cause a mess, but it fell out of the design naturally.

Both "off-main thread piping" (#97) and a similar situation with Fetch's Request class, which has a .json() method, deal with the hazards of "C++ readers" concurrent with potential "JS readers." Ideally once a stream is being read by C++, JS cannot touch it, both for performance and implementation-complexity reasons.

One solution for this might be to add some kind of "locking" mechanism so that only one consumer can read. But I don't know how to define that, without some kind of radical redesign that splits out the concept of readable stream into a stream and a stream reader or something like that.

Another possibility that would work quite well I think would be to lock the stream while it's being piped somewhere. That is, at the beginning of pipeTo, set a [[piping]] = true property; when pipeTo finishes, set [[piping]] = false. While [[piping]] is true, .read() instantly throws, saying that the stream is being piped elsewhere and cannot be read.

This would solve all of our problems that inspire this issue:

• It removes the potential author footgun of accidentally calling .read() and getting racy or unexpected results, and perturbing the pipe in progress, that @acolwell was worried about.
• Off-main-thread piping (of UA-created streams) would be able to react the same way, so that to author code it looks like any normal pipe.
• Via some sleight of hand, it solves .json(), by "explaining" .json() as piping to a concatenator stream. (See Backpressure on fetch integrated with Streams w3c/ServiceWorker#452 (comment)) Under the hood the UA will probably not implement .json() that way, but what's important is that they could, with the same author-observable behavior---so that way, .json() isn't doing anything magic.

It has a few theoretical disadvantages, but I am not sure how big of a deal they are in practice:

• It means ReadableStream.prototype.pipeTo will no longer be implementable purely on top of the public readable interface; it will be using some private [[piping]] state, and some private version of the .read() method that doesn't throw while the stream is [[piping]]. This means e.g. it doesn't apply directly to ReadableByteStreams. (Note however that it still only uses public writable stream interfaces, unless we want to lock those too...)
• It removes the ability to pipe to multiple streams in order to have the one that is most free to accept writes pick them up. This sounds rare, but funnily enough just today I was asked about a npm package to do this. It could be built on top as a library though, similar to a tee stream but without any copies.

Note that this does not disable the use case of e.g. .read()ing headers from a stream and then piping the rest of it, or piping with { preventCancel: false } and .read()ing the leftover if the destination gets closed prematurely. It only disables concurrent pipes + reads, not sequential.

Would love thoughts from @tyoshino @yutakahirano @sicking and anyone else.

[chrisdickinson]

This might be overcomplicating things, but would it be possible to introduce a separate "read lock" structure + method of sorts? Something like:

var lock = readable.getReadLock()
lock.read() // proxies original readable.read()
stream.isLocked() // true
readable.getReadLock() // throws, only one lock can be extant at a time per readable
stream.read() // throws, or is a rejected promise
lock.release()
stream.isLocked() // false
stream.read() // read works as expected

The idea being that this affords read()-based consumers of the stream the same ability to lock streams as pipeTo()-based consumers. I'm not sure if there's any communication from writable back to readable on piping, but if there is, then writables could attain exclusive locks this way too (apologies for my ignorance.)

[domenic]

That's ... really interesting. It starts down the path of "some kind of radical redesign that splits out the concept of readable stream into a stream and a stream reader" but doesn't go in nearly as crazy a direction as I anticipated. I do definitely like that it gives us a simpler primitive that pipeTo then builds on top of, and can be used for .read()-based consumers. I'll noodle on it for an hour and see what it would end up looking like.

I'm not sure if there's any communication from writable back to readable on piping, but if there is, then writables could attain exclusive locks this way too (apologies for my ignorance.)

With normal author-implemented streams no, but sometimes in UA-stream to UA-stream case there might be.

[domenic]

Wrote up spec-ish detail for @chrisdickinson's suggestion at https://gist.github.com/domenic/d421643d95cdec9a9b5b. Minor ergonomic tweaks bring it to

var lock = new ReadableStreamLock(readable)
lock.read() // proxies original readable.read()
stream.isLocked // true
new ReadableStreamLock(readable) // throws, only one lock can be extant at a time per readable
stream.read() // throws
lock.release()
stream.isLocked // false
stream.read() // read works as expected

I actually think this is strictly superior to my OP proposal because it explains the mechanism by which my proposal works and gives authors access to its power instead of reserving it for pipeTo only.

Readable stream look-alikes, like @tyoshino's ReadableByteStream, still don't have an easy time. The brand check in the ReadableStreamLock constructor prohibits it from being used with ReadableByteStream. And that in turn means that ReadableStream.prototype.pipeTo doesn't work directly when applied to a ReadableByteStream. (We could hack it to work with ReadableByteStream specifically, but we shouldn't, because we're still not solving the real problem for e.g. author-defined stream lookalikes.)

You could create a separate lock class for ReadableByteStreams? And hook it up via ReadableByteStream.Lock = ReadableByteStreamLock, and consult this.constructor.Lock instead of always using %ReadableStreamLock% in pipeTo? Is that worth it? Implementations would have to detect tampering with a readable stream's constructor or constructor.Lock properties and remove any assumptions guiding pipeTo optimizations, which is pretty complex. Is there something better?

[chrisdickinson]

Readable stream look-alikes, like @tyoshino's ReadableByteStream, still don't have an easy time. The brand check in the ReadableStreamLock constructor prohibits it from being used with ReadableByteStream. And that in turn means that ReadableStream.prototype.pipeTo doesn't work directly when applied to a ReadableByteStream. (We could hack it to work with ReadableByteStream specifically, but we shouldn't, because we're still not solving the real problem for e.g. author-defined stream lookalikes.)

Is this due to the "Brand-check that stream is a real ReadableStream." step in the ReadableStreamLock class instantiation? If so, could that be relaxed to "Check for read property pointing at function"? If the reason for the check is to get access to stream@[[locked]], could readable.isLocked be moved to ReadableStreamLock.isLocked(readable) to remove the need for ReadableStreams to have a locked slot? (also, is "slot" the correct term here, or "symbol"?)

EDIT: I just realized that even if there were no need to get to stream@[[locked]], the stream would still need to be aware of it because read() needs to throw while the stream is locked.

[domenic]

Is this due to the "Brand-check that stream is a real ReadableStream." step in the ReadableStreamLock class instantiation?

Yep

If so, could that be relaxed to "Check for read property pointing at function"?

Not quite; the main properties we use are (a) has a @[[locked]] property we can manipulate; (b) can be fruitfully passed to ReadFromReadableStream, which will do a bunch of internal state stuff (see equivalent current spec).

You can't just call this@[[stream]].read(), because ReadableStream.prototype.read() will throw if anyone tries to call it on a locked stream.

Hmm, maybe you use JS's single-threaded nature to un-lock the stream, call its .read(), then re-lock it after read completes in a finally block. That still leaves (a) though, but it's seeming more soluble...

If the reason for the check is to get access to stream@[[locked]], could readable.isLocked be moved to ReadableStreamLock.isLocked(readable) to remove the need for ReadableStreams to have a locked slot?

That is starting to sound promising... basically it maintains a side table (weak map) of whether a stream is locked. Then ReadableStream.prototype.read() can consult that side table? But how? Via this.constructor.Lock.isLocked(this) perhaps? A bit messy, but could work...

(also, is "slot" the correct term here, or "symbol"?)

Slot, or "internal slot," means "truly private state." It's a spec-only concept, but can be reified in JS (not very performantly) via a weak map.

A symbol is a JS concept, and is just a non-string-named property. It can be exposed to user code through e.g. Object.getOwnPropertySymbols, and so isn't suitable for truly private state. E.g. if we used a symbol here someone could set [[lockedSymbol]] to arbitrary values at arbitrary times. Which might usually be OK for Node code where we say "you mess with the innards, you get what's coming to you," but doesn't work when we're trying to create ironclad guarantees that allow UAs to shift work to other threads.

[domenic]

Oh, no, I just realized, with your suggestions, you can use the same side table for every type of stream; you don't need one side table per stream type. Updated gist coming shortly.

[domenic]

OK, pseudo-specced out your suggestions at https://gist.github.com/domenic/d421643d95cdec9a9b5b#file-readable-stream-locks-2-md. The key differences are that by moving to a side-table model instead of using the [[locked]] slot, and by doing the unlock-read()-lock again dance in ReadableStreamLock.prototype.read(), we can be completely generic and work with any object that has a read() method.

It's now entirely extensible. Specs that need strong guarantees can use "If %ReadableStreamLocks% has this" to guard their read() methods; author code that wants locking support can just use if (ReadableStreamLock.isLocked(this)). They are basically the same except the latter can be tampered with, which is fine because that's our usual spec-code vs. author-code division.

I'm a bit worried about the performance impact of this kind of "weak map" usage though.

[yutakahirano]

If so, could that be relaxed to "Check for read property pointing at function"?

Not quite; the main properties we use are (a) has a @[[locked]] property we can manipulate; (b) can be fruitfully passed to ReadFromReadableStream, which will do a bunch of internal state stuff (see equivalent current spec).

You can't just call this@[[stream]].read(), because ReadableStream.prototype.read() will throw if anyone tries to call it on a locked stream.

I think Readable(Byte)Stream can provide ReadFromReadableStream and unlock to the ReadableStreamLock constructor. If streams assure they expose the methods only to ReadableStreamLock, lock state check / manipulation will go away from ReadableStreamLock implementation.

[domenic]

@yutakahirano I don't quite understand what you're proposing. Would it remove the readableStream.read() method, requiring consumers to always use locks? How does it prevent multiple concurrent locks? How does it work with user-defined stream classes? (We want to avoid giving abilities only to UA-created stream classes like ReadableStream and ReadableByteStream.)

[yutakahirano]

@domenic Sorry for the poor expression.
What I thought about was:

ReadableStreamLock = (read, release, token) => {
  this.read = read.bind(undefined, token);
  this.release = release.bind(undefined, token);
};

ReadableStream.prototype.getReadLock = () => {
  if (this@[[locked]]) throw Error(...);
  this@[[locked]] = true;
  var token = this@[[token]];

  return new ReadableStreamLock((token) => {
    // read
    if (token !== this@[[token]]) throw Error(...);
    return ReadFromReadableStream(this);
  }, (token) => {
    // release
    if (token !== this@[[token]]) throw Error(...);
    this@[[locked]] = false;
    this@[[token]] = NewToken();
  }, token);
};

ReadableStream.prototype.read = () => {
  if (this@[[locked]]) throw Error();
  return ReadFromReadableStream(this);  
};

Does it make sense?

[yutakahirano]

ReadableStreamLock = (read, release, token) => {
  this.read = read.bind(undefined, token);
  this.release = release.bind(undefined, token);
};

This was wrong and should be:

ReadableStreamLock = function(read, release, token) {
  this.read = read.bind(undefined, token);
  this.release = release.bind(undefined, token);
};

[domenic]

Ah I see. Hmm, that gives the advantage that user-defined streams can create their own locking mechanism by creating their own getReadLock() function. It does seem to work pretty nicely. The token is slightly awkward but solves the problem in a clever way. I like it. Will try to prove it out a bit more, but seems good.

[yutakahirano]

It is natural to throw an exception when .read() is called on a locked stream, but what should we do when .wait() is called?
I think state change should be unobservable from the outside when locked. We will need to expose another .wait() for ReadableStreamLock as well as .read().

[domenic]

Hmm. So pipeTo actually uses .state, .ready (formerly .wait()), and .closed in addition to .read(). Do we need to proxy all of them? Eek, now it's starting to look like it's a whole stream object.

My original plan was for the stream to appear to be "waiting" whenever off-main-thread piping was going on. But I am not sure how to fit that into the lock design. Need to think on it more.

[yutakahirano]

I think it is good to proxy .state, .ready and .read.

It might be useful if closing a stream unlocks it implicitly. Then we don't need to proxy .closed.

[domenic]

I tried to prototype your idea in a bit more detail at https://github.com/whatwg/streams/blob/lock/reference-implementation/lib/experimental/exclusive-stream-reader.js. I am still uneasy about how much we are duplicating the stream interface here. Unsure what to do about it.

It might be useful if closing a stream unlocks it implicitly. Then we don't need to proxy .closed.

This would require the stream to track any locks it creates. Certainly doable, but adds a bit more complexity. Unsure if there's much gain either in preventing multiple consumers from seeing that the stream was closed or became errored.

---

I wonder if there is a simpler design we can use that doesn't require a new object that parallels the stream API. One idea would be something like

var lock = stream.lock();
stream.read(lock); // works
stream.read(); // throws
lock.release();
stream.read(); // works now

This is kind of nice since the code stays entirely within the ReadableStream class. However I don't know how to solve this for the getters, lol -_-.

[domenic]

At this point I am tempted to go back to my original idea of [[piping]] built in to the stream. It would cause .read() to throw, .state to return "waiting", and .ready to return a promise that is fulfilled when the piping is over and the stream becomes readable or closed (perhaps errored too, given #245). Everything else seems over-engineered. Which would be OK if the user-facing ergonomics were worth it, but I am not sure that they are.

[mikeal]

Having piping built in to the stream sounds tempting for some performance cases but it's only going to work if it can take a user specified read method and just make it private for use by the internal pipeing. There's all kinds of reasons you need to customize or wrap the read function. I do it in almost every new style stream I've written, usually to do lazy completion and state checking the first time read() is called.

[domenic]

I guess the disadvantage of going back to [[piping]] is that other consumers which want an exclusive lock (like the .json() function in Fetch) then need to go through contortions like piping to a backpressure-less writable stream that accumulates the data. That sucks.

[domenic]

In IRC @tabatkins made me feel better about implementing .json() in terms of piping to a writable stream that accumulates the data. Roughly his argument was that .json() is a pipe-like consumer, in that it tries to direct all the stream's data to a single place. It hides the piping from you behind a nice simple API, but underneath the covers it's basically piping.

HOWEVER, NEW IDEA:

This might be competely crazy but what if instead of duplicating .read(), .ready, and .state into the ExclusiveStreamReader, we said they were only available in the reader, and the stream itself only had pipeTo/pipeThrough (which operate via getting a reader?).

I think the implementation would be more or less the same as https://github.com/whatwg/streams/blob/lock/reference-implementation/lib/experimental/exclusive-stream-reader.js except we'd remove .ready/.read()/.state from ReadableStream.

Whether this is acceptable kind of depends on how strongly we believe that readable streams should primarily be consumed by piping them places. It'd also be a kind of big change, which I am leery of at this stage.

[yutakahirano]

It's a very interesting idea, I think.
Then WritableStream will have no properties / methods, right?
If that's correct, It might be good to have a class for transform stream and remove ReadableStream and WritableStream, and have Reader and Writer instead.

[domenic]

I didn't even think about writable stream :-/.

After sleeping on it, I think it is OK to duplicate the interface. The way @tabatkins phrased it was:

• Readable streams are normally only consumable via piping
• For lower-level exclusive access (via read()/ready/state), use a Reader.
• However, we can add convenience versions of read()/ready/state on to ReadableStream for people who don't want to be bothered.

If we make that clear in the documentation, it feels OK. I'm not sure if it'll affect the spec or implementation strategy.

The remaining issue in my mind is exactly how we create these reader objects. My spike follows your suggestion, which is pretty clever. However, I made a tweak in the last commit that makes it easier for custom stream implementers. For example let's say I want to do something like

class InstrumentedReadableStream extends ReadableStream {
  read() {
    const chunk = super.read();
    console.log('chunk!', chunk);
    return chunk;
  }
}

In your design, any reads made through the lock would not trigger the instrumentation, since it delegates directly to ReadFromReadableStream. With my tweak, it delegates to this.read after temporarily unlocking the stream, which means that custom overriden reads work.

---

Overall I am feeling better about this. A few minor issues we should think about:

• Can we refine the setup so that the code inside getExclusiveReader() is not so full of boilerplate?
• The name might not need "exclusive"; that could be left implicitly understood.
• Should we add .closed to the reader too? Probably, I think.
• Is it a good or bad idea to try to flip the implementation along the conceptual lines earlier in my message? For now it is probably a bad idea since creating a lock is a relatively heavyweight operation, but maybe if we tweak the implementation it can be a simple object allocation without much complicated setup, which sounds possibly acceptable?
• My setup in https://gist.github.com/domenic/d421643d95cdec9a9b5b#file-readable-stream-locks-2-md, if properly extended to ready/state as well, was kind of simple. It's similar to the current situation except it uses a shared weak set to let the reader access the stream, instead of having the stream hand the reader those capabilities at creation time. I am curious how implementers feel about the two different approaches.

[domenic]

Can we refine the setup so that the code inside getExclusiveReader() is not so full of boilerplate?

I think I did this via the latest commit to my spike.

I am curious how implementers feel about the two different approaches.

I sent https://readable-email.org/list/public-script-coord/topic/weak-set-usage-in-streams to get some opinions.

[yutakahirano]

However, we can add convenience versions of read()/ready/state on to ReadableStream for people who don't want to be bothered.

That is an idea of proxying read(), .ready and .state, right? Then I think introducing a pseudo-state "locked by someone" is a good way.

[yutakahirano]

• ReadableStream itself has a (private) reader.
• Each of ReadableStream's .read(), .ready and .state calls the corresponding method of ReadableStream's reader.
• .pipeTo() and .getReader() acquire a lock. They succeed only when ReadableStream' reader is locked. The reader will be released automatically.
• When a reader other than ReadableStream's is released, ReadableStream's reader will be locked again automatically.
• When .read() is called through a released reader, it throws.
• When a reader is released, its .ready is resolved.
  • It might be good that when closed or errored the promise gets reset, but I'm not sure.
• When a reader is released, its .state returns "locked by someone".
  • It might be good that "closed" and "errored" can be seen via an unlocked lock, but I'm not sure.

Makes sense?

[domenic]

Makes sense, although I will need to try validating to make sure that things like

When a reader other than ReadableStream's is released, ReadableStream's reader will be locked again automatically.

Are doable in a sane way, and to figure out the "not sure" things. Will try to do so Monday.

When a reader is released, its .state returns "locked by someone".

I think "waiting" works fine, as the observable behavior is the same---you cannot read or see anything happen until .ready fulfills.

[yutakahirano]

At least for ReadableStream, checking if the lock is acquired is useful, I think. Having a boolean property (.readable or .locked ?) on ReadableStream is one idea and having a boolean property (.released) on Reader is another idea. The latter is somewhat similar to introducing a new pseudo-state.

[yutakahirano]

friendly ping

[domenic]

Heh, thanks. I've been working through this all day actually. Last week was spent on some V8 work, sorry. Here is some stuff I wrote up:

https://github.com/whatwg/streams/blob/lock/reference-implementation/lib/experimental/locking.md

Currently I am trying to work out all the implications of exactly how to specify this. I think the work at

https://github.com/whatwg/streams/blob/lock/reference-implementation/lib/experimental/exclusive-stream-reader.js

is pretty OK, although there are a few issues:

• I want to make sure it is optimizable. As-is, the try/catch surrounding all the delegating methods/getters of the reader is overhead. Implementations should ideally be able to optimize, at least in the non-subclass case. I want to outline a strategy for doing so, even if that isn't spec-level material. It will probably look similar to your idea of a stream having a default reader. (However, as explained in the linked doc, that isn't very friendly to subclasses: for subclassing to work seamlessly, StreamReader should delegate to ReadableStream, instead of ReadableStream delegating to StreamReader.)
• If we say that custom readable stream implementations must do their own locking mechanism without our help, then we can simplify it a bit (remove getToken/setToken, and probably that means we can do new StreamReader(stream) instead of stream.getExclusiveReader()). I think I am OK with that since custom implementations already have to do so much work, and subclassing will give you most of the benefits. (See the linked writeup for more details on the differences here.)
• Need to nail down some naming
• Need to figure out how to do the right semantics for ready and state.

Anyway this will be my focus for the next day or two; it's getting to be end of day here so tomorrow is likely when most of the work will be done. I aim to have a pull request up for discussion by EOD tomorrow.

[tyoshino]

Took a look at Locking Design Doc.md. Almost all of the rationale looks good to me.

I agree that there's no compelling reason for exposing stream.isLocked, yet.

lock.isActive looks useful as Yutaka said (#241 (comment)) but I also wonder if there's any concrete use case where this is required (not just useful).

releaseLock() maybe to make it more descriptive, you've included Lock. Right? Just release() would have any problem?

Even after release, reader's closed, ready, and state getters will simply delegate to the encapsulated stream as the note at https://streams.spec.whatwg.org/#reader-release-lock explains. I don't have strong opinion yet but wonder if this is useful or harmful.

[domenic]

Thanks for the review @tyoshino; sorry I didn't get a chance to pass it by you earlier.

lock.isActive looks useful as Yutaka said (#241 (comment)) but I also wonder if there's any concrete use case where this is required (not just useful).

Yeah, maybe we should remove it until someone asks for it?

I guess part of the reason I have it is that it's better to avoid forcing people to use try/catch. I can't think of a real world situation where you would, but it seems nicer to be able to do

if (reader.isActive && reader.state === "readable") {
  console.log(reader.read());
} else {
  console.log("not locked!");
}

instead of

if (reader.state === "readable") {
  try {
    console.log(reader.read());
  } catch (e) {
    console.log("not locked! or maybe another error?", e);
  }
}

releaseLock() maybe to make it more descriptive, you've included Lock. Right? Just release() would have any problem?

I said releaseLock() since usually the variable will be reader, not lock. I.e., reader.releaseLock(). I also think this is a bit helpful to differentiate operations that pass through to the stream (like read() and cancel()) versus ones having do to with the reader and the lock it has acquired. Just release() sounds too similar to cancel() for me.

Even after release, reader's closed, ready, and state getters will simply delegate to the encapsulated stream as the note at https://streams.spec.whatwg.org/#reader-release-lock explains. I don't have strong opinion yet but wonder if this is useful or harmful.

I at first had a design where they would fail but it turned out to be quite hard to write pipeTo correctly in that design, because pipeTo consults those properties a lot. For example in the design where closing a stream automatically releases any held locks, the check source.state === "closed" never works since by the time it's closed state becomes a throwing getter. If I recall even in designs without auto-release-on-close there were other problematic cases as well. So I think it is pretty useful in the end.

[tyoshino]

I was on vacation and didn't have time to do it, sorry. Thanks so much for great progress!

I guess part of the reason I have it is that it's better to avoid forcing people to use try/catch.

Oh, I see. I'm now a little inclined to keep it.

I said releaseLock() since ...

Got it!

the check source.state === "closed" never works

Thanks for explanation. I understand and agree that it makes sense for transition-to-"closed" case.

When a reader is unlocked while the associated stream is readable/waiting, it sounds a bit weird to keep exposing the non-closed stream's status via the released reader. What do you think about making a reader look like a closed stream regardless for what kind of transition the unlock happened?

[tyoshino]

I.e. making

• ready return a resolved promise
• state return "closed"
• closed return a resolved promise

or keep it look waiting?

Ah, for errored case, we should make the reader look errored, sorry.

[domenic]

Hmm, that's interesting. I think it is better than what we have now, yes. I will open an issue to take care of it.