Ensure the JS thread doesn't have to get hit when piping two streams to each other

[domenic]

E.g., an XHR stream to a file system stream, xhrStream.pipeTo(fsStream).

As-is, there might be a problem if e.g. the user does

var oldWrite = fsStream.write;
fsStream.write = function (chunk) {
  console.log("AOP logging!!", chunk);
  oldWrite.apply(this, arguments);
};

after the pipe is already set up. The implementation would have to detect this and start routing data to the main thread, which kind of sucks.

A possible fix would be caching the value of dest.write at the top of pipeTo.

/cc @sicking

[sicking]

One important question is if we actually want to support code like the above?

My concern is that it makes it very easy to create footguns where you suddenly get dramatically slower performance for something that is seemingly a harmless no-op.

For example

var oldWrite = fsStream.write;
fsStream.write = function (chunk) {
  if (debug)
    console.log("saved", chunk);
  oldWrite.apply(this, arguments);
};

or even

fsStream.write = fsStream.write.bind(fsStream)

I don't know the answer. But it's not obvious to me that it's beneficial that overriding write affects pipeTo.

[domenic]

The issue is that if pipeTo doesn't go through a public API like write then it is impossible for users to create streams themselves; they can only use platform-provided streams.

[tyoshino]

users to create streams themselves

This is important, but I don't think we need to enable replacing write of dest with ongoing piping.

We should also clearly specify how pipeTo determines whether to use such an offloaded/platform piping or call JS function.

[domenic]

The base pipeTo should always follow the specified algorithm; subclasses can always override it with more specialized versions.

[tyoshino]

The base pipeTo should always follow the specified algorithm; subclasses can always override it with more specialized versions.

I meant how to check if dest can receive piped data out of JS thread or not. If dest's write is overridden by JS code, it needs to be run in JS thread. Otherwise, we could do some optimized offloaded pipe depending on dest's type.

[domenic]

Well, my understanding of @sicking and others' ideas was that if you did certain special combinations, e.g. xhrStream.pipeTo(fsStream), then XHRStream.prototype.pipeTo would recognize it's dealing with an FSStream, and do off-main-thread piping work. That is, something like

XHRStream.prototype.pipeTo = function (dest, options) {
  if (dest.[[InternalStreamType]] === "file") {
    cppBindings.pipeTogetherOffMainThread(this, dest, options);
  } else {
    ReadableStream.prototype.pipeTo.apply(this, arguments);
  }
};

[domenic]

Er, wait, I realize that doesn't address what you're talking about. Yes, I agree, determining clearly what the criteria are is important... the spec should provide some hook other specs can use so that they can do e.g.

XHRStream.prototype.pipeTo = function (dest, options) {
  if (dest.[[InternalStreamType]] === "file" && isUndisturbed(dest)) {
    cppBindings.pipeTogetherOffMainThread(this, dest, options);
  } else {
    ReadableStream.prototype.pipeTo.apply(this, arguments);
  }
};

Here isUndisturbed is the hook. It wouldn't be an actual function (I don't think?), but just an algorithm other specs could refer to.

---

This is all of course assuming our solution is indeed of the caching-write flavor described. Other solutions would be some kind of double-dispatch, which we could conceivably even bake into the base spec... e.g.

ReadableStream.pipeTo = function (dest, options) {
  if (dest.pipeFrom(this)) {
    // successfully hooked up at the OS level; all done
  } else {
    // the dest had no special way of hooking itself up to `this`, so do the usual algorithm
  }
};

[tyoshino]

Right. Thanks.

One more point I'd like to clarify is how dest's state getter should work during offloaded pipeTo() is working. I guess the right value to return in such a case (not yet closed) is basically "waiting".

[ferasm]

I like the concept of isUndisturbed.

I also agree it would not be an exposed function, and would like to have it be well defined in the spec - I can see other specs needing this behavior documented and will be able to reference this spec for a definitive algorithm.

@tyoshino - I don't quite understand why the state getter should behave differently?

[tyoshino]

@ferasm state getter is the only synchronous (closed getter is also, but what it returns is the constant reference to the [[closedPromise]] instance) method on BaseWritableStream. So, I wondered how it should behave while pipe algorithm is running asynchronous to the JS thread. We should also discuss how the other methods (except for closed. It's clear) should behave but I was especially concerned with state.

[othiym23]

/cc @Gozala