Rigorously specify and test pipeTo #512

domenic · 2016-08-30T20:45:12Z

This is a first draft at the "algorithm" for pipeTo, which contains a few concrete steps and a lot of more general requirements. I'd love feedback on whether this style of speccing is clear enough, or if I missed any important requirements, or if the way in which I grouped the requirements is confusing, or anything else. I'm kind of nervous about this part :)

Preview at https://streams.spec.whatwg.org/branch-snapshots/spec-pipe-to-omg/#rs-pipe-to

Things to do before this is merge ready:

~~Factor this out into a StreamPipe() abstract op that other specs could use. (ReadableStreamPipe??)~~ Let's wait until we have consumers first
Factor our releasing a writable stream lock into an abstract op
Ensure we have tests that cover all of the requirements here (and realign any existing ones to match better with the requirements' wording if they are close)
Ensure we are not missing any requirements that are expressed in the tests
Ensure in particular that the various error conditions are tested correctly and align with the spec, e.g. what if both the source and dest error.
Check to make sure no asserts are violated or incorrect errors are thrown/promises are rejected, e.g. by doing something that's not supposed to be done while in a given state.
Align the reference implementation more closely with this version of the spec, by using more internal methods and seeing if we can get better correspondence between the requirement lines here and the JS lines in the reference implementation. It's possible they can't be that close, which is fine, as long as the tests pass.

This will help with specifying pipeTo, as seen in #512. This also aligns the spec, tests, and reference implementation on sharing a single error object across both closed and ready promises; previously this was untested and the reference implementation did so but the spec did not.

ricea · 2016-08-31T02:24:51Z

I like this style. I find it easy to read and easy to write tests for.

This will help with specifying pipeTo, as seen in #512. This also aligns the spec, tests, and reference implementation on sharing a single error object across both closed and ready promises; previously this was untested and the reference implementation did so but the spec did not.

tyoshino · 2016-09-06T05:33:15Z

index.bs

+         new *TypeError*.
+     * <i>Cleanup</i>: if any of the above requirements ask to perform cleanup, then:
+       * Wait until any outstanding reads or writes finish (i.e. the corresponding promises settle). If that causes one
+         of the above error conditions to be true, jump back to those requirements instead of proceeding with cleanup.


Depending on resolution of #476, we might need to mention the error situation where write() promise rejects but the destination writable stream doesn't get errored.

tyoshino · 2016-09-06T06:16:48Z

Regarding that all the operations are performed by directly invoking the abstract operations:
- I think this is great.
The way the algorithm is specified:
- Yeah, it's nice. The algorithm in the reference implementation is also written in the form reacting to each situation. We just have state to resolve race. The spec text is representing what we have well :)
reviewed the code changes. left some comments but lgtm.

domenic · 2016-09-07T23:34:36Z

Update: I made a lot of progress on this today. In the process I discovered some contradictions in the spec text as written, which was kind of expected. E.g. races between the various error conditions, or races between performing cleanup (now renamed shutdown) and finishing a write. The spec has been amended, and also the reference implementation has been rewritten to follow the spec.

There are still a number of failing (unconverted) tests, so I guess the algorithm is probably not quite right yet.

I was still not able to remove many existing tests because we have a lot of good tests that manage to put a stream in a given state asynchronously, possibly after some chunks have already passed through it. Whereas the new tests I've written so far all put the stream in a given state synchronously or use a rejected promise returned from the start() function. I am not sure yet whether I want to solve this in some kind of generative way (maybe like the templated tests) or just by porting the existing ones individually. I'll try to work on it tomorrow.

This will help with specifying pipeTo, as seen in #512. This also aligns the spec, tests, and reference implementation on sharing a single error object across both closed and ready promises; previously this was untested and the reference implementation did so but the spec did not.

In the process of testing this, ported several tests to web-platform-tests format, and introduced the recording stream helpers that I am using extensively in #512.

domenic · 2016-10-06T23:48:13Z

This is allllmost done. Some TODOs listed in the commit message, all minor. Last time I said "tomorrow" in this thread that didn't work out, but I'm hopeful...

ricea · 2016-10-07T04:36:49Z

reference-implementation/to-upstream-wpts/resources/test-utils.js

@@ -0,0 +1,43 @@
+'use strict';
+
+self.getterRejects = (t, obj, getterName, target) => {


I would prefer assertGetterRejects(), to make it clear that this is an assertion.

I thought about assert_getter_rejects(), but then people might expect to find the definition in testharness.js.

I realise that testharness's "promise_rejects" doesn't include "assert", but I find that a bit counter-intuitive to be honest.

This whole file already exists upstream; I just am copying it here because of limitations of the test runner.

I see. Never mind then.

domenic · 2016-10-07T21:43:18Z

This is done and ready for review! ~~It's on top of #526 so the first two commits can be ignored.~~ Those commits are now in master so this PR only contains a single commit again.

In the process of testing this, ported several tests to web-platform-tests format, and introduced the recording stream helpers that I am using extensively in #512.

domenic · 2016-10-21T22:24:21Z

Can't that be done without involving the whole pipeLoop? It doesn't really need all the shutdown infrastructure does it.

It could. I find the spec and reference implementation cleaner this way.

As we can see the source clearly based on which value we passed to the prevent.* flag, I don't strongly object to this now, but some note needed to explain this behavior.

Added some bullet points

please add some tests where two or more of prevent.* flags are set to true.

Hmm. Given that only one flag can be used in any given pipeTo, I'm not sure how valuable this would be. Could you give an example of what you'd be hoping to test?

Although, I guess I just realized we do not have any tests for multiple close/error conditions being true at once :(. So e.g. piping a closed readable stream to an errored writable stream and so on. The spec isn't even super clear about what takes precedence there. I guess that's another reasonably large chunk of work to do. I'll try to do it Monday.

tyoshino · 2016-10-24T10:36:37Z

Hmm. Given that only one flag can be used in any given pipeTo, I'm not sure how valuable this would be. Could you give an example of what you'd be hoping to test?

E.g. please try simply adding preventCancel: true to the Errors must be propagated forward: starts errored; preventAbort = ${String(truthy)} (truthy) test in error-propagation-forward.js and see how many times shutdown() is invoked. It's invoked twice and finalize() is also invoked twice. The second finalize() fails harmlessly on the redundant call of WritableStreamDefaultWriterRelease() which throws. It's not affecting the publicly visible behavior, but I think we should suppress the redundant invocation of finalize().

That's the background of my comment at #512 (comment).

In general, I think it's worth testing some cases where two or three of prevent.* flags are set to a truthy value (though no need to have full coverage) as it's possible and should be common to do so, in addition to what you've done which is, of course, needed and important.

Although, I guess I just realized we do not have any tests for multiple close/error conditions being true at once :(. So e.g. piping a closed readable stream to an errored writable stream and so on. The spec isn't even super clear about what takes precedence there. I guess that's another reasonably large chunk of work to do. I'll try to do it Monday.

Agreed.

domenic · 2016-10-24T14:22:41Z

It's not affecting the publicly visible behavior, but I think we should suppress the redundant invocation of finalize().

Why? The entire setup is designed specifically so that it's harmless and doesn't affect public behavior; we shouldn't need to do any extra bookkeeping for suppression.

tyoshino · 2016-10-25T05:10:30Z

Why? The entire setup is designed specifically so that it's harmless and doesn't affect public behavior; we shouldn't need to do any extra bookkeeping for suppression.

Okay. I still think we should have the if-clause and some tests with multiple prevent.* set to true, but it shouldn't block this from landing. lgtm. I'll open an issue for that.

tyoshino · 2016-10-25T06:52:40Z

I'll open an issue for that.

Created. Let's discuss this there.
#557

ricea · 2016-10-25T08:38:29Z

lgtm based on my own superficial review and tyoshino's in-depth lgtm.

domenic · 2016-10-25T21:45:48Z

I have to leave early today unfortunately. I want to get some good tests for multiple error/closure scenarios before merging this. So I've pushed up some WIP ones that are mostly failing. The tests might be bad or the spec or the logic; not sure yet. No need to pay them much attention unless you have extra free time. Just wanted to give an update.

I think we can also resolve #557 before merging this too.

This commit introduces an actual spec for pipeTo, now that writable streams are nailed down. The algorithm explicitly notes that much of the actual data movement is unobservable due to locking, and as such just specifies some requirements (mostly around error and close propagation) for how the piping must happen. The reference implementation pipeTo is rewritten to more explicitly reflect this algorithm and its requirements. All pipeTo tests have been converted to web platform tests format, with cleanups as that was done. Some coverage was lost by not using the templating machinery, but some was gained by testing the options more exhaustively in various scenarios. One flow control test (the complicated one with the many different times at which things are tested) changed in expectation, in a positive way. We now explicitly prohibit reading while the writable stream is not ready, whereas the previous algorithm would read a chunk from the readable stream and keep it in "limbo" between the two streams. The new formulation propagates backpressure better; the readable stream's queue is not alleviated prematurely, so backpressure propagates more reliably.

This avoids an extra "wait until any ongoing writes finish".

…seWithErrorPropagation

domenic mentioned this pull request Aug 30, 2016

Factor out WritableStreamDefaultWriterRelease #513

Merged

domenic force-pushed the spec-pipe-to-omg branch from 8d972ed to 3ad0eeb Compare August 30, 2016 21:14

domenic mentioned this pull request Aug 30, 2016

Convert writable stream tests to web platform tests format #492

Closed

5 tasks

domenic force-pushed the spec-pipe-to-omg branch from 3ad0eeb to f8ae6ef Compare September 1, 2016 23:10

tyoshino reviewed Sep 6, 2016
View reviewed changes

domenic mentioned this pull request Sep 6, 2016

Add a proof of concept of optimized pipe #511

Open

domenic force-pushed the spec-pipe-to-omg branch from f7767a7 to 5696093 Compare September 9, 2016 19:13

domenic mentioned this pull request Oct 6, 2016

Reject writer.ready promise on errors, instead of fulfilling #526

Merged

domenic force-pushed the spec-pipe-to-omg branch from 5696093 to 798a7b6 Compare October 6, 2016 23:47

ricea reviewed Oct 7, 2016

View reviewed changes

domenic force-pushed the spec-pipe-to-omg branch from 798a7b6 to 42fa349 Compare October 7, 2016 21:40

domenic changed the title ~~First draft at a pipeTo "algorithm"~~ Rigorously specify and test pipeTo Oct 7, 2016

domenic force-pushed the spec-pipe-to-omg branch from 42fa349 to 1f32890 Compare October 10, 2016 18:01

domenic force-pushed the spec-pipe-to-omg branch from 1f32890 to 119449f Compare October 12, 2016 01:42

domenic assigned tyoshino and ricea Oct 12, 2016

domenic mentioned this pull request Oct 12, 2016

Port some TransformStream tests to wpt #531

Merged

tyoshino mentioned this pull request Oct 25, 2016

pipeTo(): Rationale for not making shutdown() no-op when shuttingDown is set #557

Closed

tyoshino mentioned this pull request Oct 25, 2016

pipeTo(): Rationale for propagating an error happened during error propagation to pipePromise #558

Closed

domenic force-pushed the spec-pipe-to-omg branch from b7f5663 to f65451f Compare October 25, 2016 21:44

domenic added 15 commits October 27, 2016 18:20

Async shutdown -> shutdown with an action

e00e2aa

Further wording tweaks for shutdown: get rid of "perform"

abd3f40

Factor out "finalize" per Takeshi's review

db327dd

This avoids an extra "wait until any ongoing writes finish".

Assert never closing/closed when doing WritableStreamDefaultWriterClo…

d341f8c

…seWithErrorPropagation

Fix unnecesssary exports per isonmad's feedback

ccdcc5b

Reject instead of throwing for locked streams (isonmad's feedback)

8ab4ed1

ToBoolean cannot throw

51da7f5

Simplify reference implementation close-propagation-backward

501972f

Address review comments

7fecbaf

Add explanatory notes

3b6870f

Link promise terms

6a55042

Add multiple propagation tests

25c7b65

Fix broken tests

d14419f

Fix according to new tests

00e7ebf

domenic force-pushed the spec-pipe-to-omg branch from f65451f to b363d00 Compare October 28, 2016 21:40

Add tests with additional options set

ad3aaaf

domenic force-pushed the spec-pipe-to-omg branch from b363d00 to ad3aaaf Compare October 28, 2016 21:43

domenic merged commit fd44398 into master Oct 28, 2016

domenic deleted the spec-pipe-to-omg branch October 28, 2016 21:52

		@@ -0,0 +1,43 @@
		'use strict';

		self.getterRejects = (t, obj, getterName, target) => {

Rigorously specify and test pipeTo #512

Rigorously specify and test pipeTo #512

Uh oh!

Conversation

domenic commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ricea commented Aug 31, 2016

Uh oh!

tyoshino Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

tyoshino commented Sep 6, 2016

Uh oh!

domenic commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic commented Oct 6, 2016

Uh oh!

ricea Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

domenic Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

ricea Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

domenic commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic commented Oct 21, 2016

Uh oh!

tyoshino commented Oct 24, 2016

Uh oh!

domenic commented Oct 24, 2016

Uh oh!

tyoshino commented Oct 25, 2016

Uh oh!

tyoshino commented Oct 25, 2016

Uh oh!

ricea commented Oct 25, 2016

Uh oh!

domenic commented Oct 25, 2016

Uh oh!

Uh oh!

domenic commented Aug 30, 2016 •

edited

Loading

domenic commented Sep 7, 2016 •

edited

Loading

domenic commented Oct 7, 2016 •

edited

Loading