In pipeTo spec, the order of checking for shuttingDown disagrees with ref. impl.

[zenmumbler]

In the spec for pipeTo at https://streams.spec.whatwg.org/#rs-pipe-to at the Shutdown and Shutdown with an action sub steps, the ordering is a) wait for writes b) check shuttingDown c) set shuttingDown to true.

The reference implementation implements this as: b, c, a, that is first check if shuttingDown was set and only do the rest if not. See: https://github.com/whatwg/streams/blob/master/reference-implementation/lib/readable-stream.js#L216

I found this while making my own implementation of the streams standard, (see https://github.com/stardazed/sd-streams) By changing this order, a few of the wpt/streams/piping tests fail I believe. More generally, while the pipeTo spec text claims there is some leeway in the implementation, some tests rely on an exact ordering of Promise resolves that I could only fix by stepping through and precisely emulating the reference implementation.

Anyway, my implementation is very close to the exact spec and passes all tests, except for the detached buffers for obvious reasons. I made it mostly to use the full streams spec in my browser based framework but perhaps it'll be useful to someone here or elsewhere. Thanks.

[ricea]

It looks like you're right. I haven't had time to update Chromium's implementation to have the correct semantics yet, so yours is probably the first independent implementation.

Needless to say, it shouldn't be necessary to reverse-engineer the reference implementation to get pipeTo() correct. Either the tests are too strict or the standard text is too vague.

If you have the list of tests whose requirements are stricter than the standard, that would be super useful.

Feedback from implementers is incredibly valuable, so thank you very much!

[zenmumbler]

Hi Adam, thanks for looking into this. I will go back to and make a list of the tests that confused me after I have a bit of a streams detox. I made this implementation in one go over a couple of days and need a few moments of not going over spec minutia ;)

I will say upfront that this was the first time I implemented a (whatwg) spec so my expectations may just have been… uncalibrated. Some tests seemed very strict, but perhaps that is normal. I will report back soon.

[ricea]

In general, the tests are strict so that all visible behaviour in all browsers conforms to the standard. We're trying to avoid the situation where authors end up relying on browser-specific quirks which others browsers then have to reverse-engineer to be compatible.

pipeTo() is a special case, as we want to allow browsers a lot of latitude to optimise it in ways that are appropriate for their architecture. The tests should enforce what is required by the standard, no more, no less. The tests should enforce exactly what is required by the standard, no more, no less. Hitting this mark precisely is tricky.

[zenmumbler]

Yes, that makes sense and I really only had some guesswork with the pipeTo tests, the other tests were helpful in finding issues I had forgotten, without the WPT I would have had many subtle bugs, so thank you for making those.

You asked for a list of tests and my comments on them, should I just post that here in a comment or make a new issue or issues for those?

BTW: in order to get back into streams I developed a little further and made a polyfill for streams including a fetch adapter as the Response class is where most ReadableStreams in web apps are originated or end up. This makes it easier for me to experiment with my implementation and report on things I noticed. I'm using MDN's example projects as an example workflow. Maybe it's useful for others as well. See: https://github.com/stardazed/sd-streams-polyfill

[ricea]

You asked for a list of tests and my comments on them, should I just post that here in a comment or make a new issue or issues for those?

A new issue would be the easiest thing for us to manage.

BTW: in order to get back into streams I developed a little further and made a polyfill for streams including a fetch adapter as the Response class is where most ReadableStreams in web apps are originated or end up.

That is so cool. Did you find a way to making streaming upload work? I don't think any browser does that yet, it would be great to have an implementation.

I just noticed that it's all typed using TypeScript. Also very cool.

Like the sd-streams README says, sometimes in polyfills it is better to be pragmatic than to aim for 100% conformance.

(from pipeThrough() implementation)

// not sure why the spec is so pedantic about the authenticity of only this particular promise, but hey

pipeThrough() can operate on an object that is not a real ReadableStream and call a pipeTo() method that doesn't return a real Promise. Something like this:

ReadableStream.prototype.pipeThrough.call({
  pipeTo() { 
    return { 
      then() { console.log('not a real Promise'); }
    }; 
  }
}, { readable: {}, writable: {} });

This is the only case where we set [[PromiseIsHandled]] to true on a Promise that we did not ourselves create, and so we need to be careful it actually is a real Promise before we start poking at its internals.

[zenmumbler]

Ah, thanks for explaining that Promise. That is actually one of the test cases that I can probably let fail as in JS-land (which is my informal name for the browser client execution context or… I don't really know the name) I can't set any internal Promise fields anyway. I think I was confused as I'm trying to understand what the use case would be of passing in a mock ReadableStream returning a not-promise. But good to know.

As for uploading files with my adapted fetch function: that is actually one part I didn't do yet. I can do this two ways: 1) read the entire stream, then have fetch do it's thing with the resulting Blob, or 2) actually send data as it comes in. I am leaning towards option 1.

I looked at the fetch spec when I was adding support for the Response body fetch reading and now looking back, I'm interpreting what the way to go is.
a) In the Request class section in step 36, it says "extract the body", which for a ReadableStream comes down to "get the stream object."
b) step 39 has a big red issue in it, but wait, there it is:
c) the transmit body section, yep send the bytes as they come in. I don't think with public interfaces I can send data in chunks, XHR can't (well, not until it supports ReadableStream…)

I can and will — I overlooked this — add stream uploads to fetch, but only in the "read it all on the client and send it in one go" method. I can't use XHRs or WebSockets to do streamed upload without the browser cooperating, unless there is some other way.

This issue is derailing a bit, I will add a new issue with the test cases. Thanks for the feedback!

[zenmumbler]

Just FYI, I updated the fetch-adapter and polyfill packages with support for ReadableStreams as body for fetch uploads. I added a simple test to the polyfill repo as well. It seems to work well. It uses the read it all and send as a blob method I wrote about above.

[ricea]

My initial impression is that the reference implementation is right and the standard text is wrong. I'm going to have to investigate further why changing the implementation to match the standard causes asserts to fire. If the standard text is wrong I want to have a robust understanding of why it is before I change it.

[ricea]

I was able to get the reference implementation to match the standard text and pass the tests. But I did it by adding a new variable, stopLooping, which performed the function of stopping reading that shuttingDown used to perform.

I essentially had to pretend that

if this.[[state]] is or becomes "closed", then ...

means

if this.[[state]] is or becomes "closed", then ... unless you already performed these steps, in which case do nothing.

This has made me more convinced that we need to change the standard text to set shuttingDown before waiting for pending writes, not afterwards.

[zenmumbler]

@ricea Yes, that also makes a lot more sense to me. When implementing it, I actually wondered why I should re-initiate a check for pending writes every time shutDown is entered as that would start redundant work. Moving the check for shuttingDown to the top is logical, as otherwise it's just going wait multiple times for the same writes to finish.