Add a brand check to pipeThrough() #966

ricea · 2018-11-22T14:53:41Z

Require pipeThrough() to be called on a ReadableStream. Stop delegating to
this.pipeTo().

Also throw exceptions if this or writable are not streams or they are
locked, or if signal is not an AbortSignal. Also require readable to be a
ReadableStream.

Add a new abstract operation, ReadableStreamPipeTo(), which is used internally
by pipeTo() and pipeThrough() and also can be referenced from other
standards.

Closes #961.

Preview | Diff

Require pipeThrough() to be called on a ReadableStream. Stop delegating to this.pipeTo(). Also throw exceptions if *this* or _writable_ are not streams or they are locked, or if *signal* is not an AbortSignal. Also require _readable_ to be a ReadableStream. Add a new abstract operation, ReadableStreamPipeTo(), which is used internally by pipeTo() and pipeThrough() and also can be referenced from other standards. Closes whatwg#961.

domenic

LGTM with nits... will check the tests to ensure they test the changed semantics.

domenic · 2018-11-26T15:06:28Z

index.bs

@@ -661,31 +661,31 @@ option. If <code><a for="underlying source">type</a></code> is set to <code>unde
 </div>

 <h5 id="rs-pipe-through" method for="ReadableStream" lt="pipeThrough(transform, options)">pipeThrough({
-<var ignore>writable</var>, <var ignore>readable</var> }, <var ignore>options</var>)</h5>
+<var ignore>writable</var>, <var ignore>readable</var> }, { <var>preventClose</var>, <var>preventAbort</var>,
+<var>preventCancel</var>, <var>signal</var> } = {})</h5>


Also update the class definition to use this style?

domenic · 2018-11-26T15:08:33Z

index.bs

+     _preventCancel_ to ! ToBoolean(_preventCancel_).
+  1. If _signal_ is not *undefined*, and _signal_ is not an instance of the `<a idl>AbortSignal</a>` interface, throw a
+     *TypeError* exception.
+  1. If ! IsReadableStreamLocked(*this*) is *true*, throw a *TypeError* exception.


Interesting that these now throw sync exceptions instead of returning a rejected promise that gets ignored. This is probably the right thing to do...

The alternative would be to error readable. That would have the benefit that the whole pipe chain would be errored in the normal case, and so you'd get a rejection rather than an exception. But it wouldn't work properly if readable was locked. It also would be a big departure from our behaviour up until now of passing through readable unchanged.

On balance, I think throwing an exception here is the best approach.

Agreed, sync exceptions make the most sense. WebIDL says that you should return a rejected promise if the function's return type is a promise, and otherwise you should throw an exception. Since pipeThrough returns a ReadableStream, sync exceptions would be the correct choice. 🙂

The alternative would be to error readable.

This seems like a bad alternative, for all the reasons you've mentioned. It would be very unexpected that a wrong usage of the stream would cause it to error.

domenic · 2018-11-26T15:27:23Z

When merging this we should be sure to file browser bugs; see https://github.com/whatwg/meta/blob/master/MAINTAINERS.md#handling-pull-requests

Reflect the new definition of pipeThrough

ricea · 2018-11-27T07:53:21Z

When merging this we should be sure to file browser bugs; see https://github.com/whatwg/meta/blob/master/MAINTAINERS.md#handling-pull-requests

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=908747 for Chrome. Edge and Firefox don't have implementations. It would probably not be helpful to file issues saying "implement pipeThrough". Safari appears to have a semi-functional pipeThrough that delegates to a non-functional pipeTo. I've no idea what to do about that.

domenic · 2018-11-27T14:14:15Z

LGTM, merge at will after the tests. I forgot about the implementation status, so yeah, probably it's OK to not do the bugs...

…14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966.

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193 UltraBlame original commit: fcc10971a6e8c63b0ae33f833e472ba7459fd202

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193 UltraBlame original commit: e1782a85a325a130ee1dd69b603f3015d5090ed1

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193 UltraBlame original commit: fcc10971a6e8c63b0ae33f833e472ba7459fd202

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193 UltraBlame original commit: e1782a85a325a130ee1dd69b603f3015d5090ed1

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193 UltraBlame original commit: fcc10971a6e8c63b0ae33f833e472ba7459fd202

…ough() having a brand check, a=testonly Automatic update from web-platform-tests ReadableStream: modify tests for pipeThrough() having a brand check (#14193) Remove tests that ReadableStream.prototype.pipeThrough operates generically on its arguments. Add tests that it performs brand checks on |this|, |readable| and |writable|, and throws for other precondition failures. Also verify that it *doesn't* call pipeTo(). Also verify that preventClose, preventCancel and preventAbort work, since we can no longer test that it passes through to pipeTo(). Also change the expected length of the function from 2 to 1 to reflect that the options argument is optional. Corresponding standard changes are at whatwg/streams#966. -- wpt-commits: 4606e75ca8cd69830223f02e0fbd46fc160f431f wpt-pr: 14193 UltraBlame original commit: e1782a85a325a130ee1dd69b603f3015d5090ed1

domenic approved these changes Nov 26, 2018

View reviewed changes

MattiasBuelens mentioned this pull request Nov 26, 2018

Fix memory leak in pipe loop reference implementation #968

Merged

MattiasBuelens approved these changes Nov 26, 2018

View reviewed changes

Update the class definition for ReadableStream

781a711

Reflect the new definition of pipeThrough

ricea force-pushed the pipethrough-brand-check branch from 4d98b9e to 781a711 Compare November 27, 2018 07:15

Update wpt pointer

5f1e5ef

Update web-platform-tests to landed commit

1102b84

ricea merged commit b494f35 into whatwg:master Nov 27, 2018

ricea deleted the pipethrough-brand-check branch November 27, 2018 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a brand check to pipeThrough() #966

Add a brand check to pipeThrough() #966

ricea commented Nov 22, 2018 •

edited by pr-preview bot

Loading

domenic left a comment

domenic Nov 26, 2018

ricea Nov 27, 2018

domenic Nov 26, 2018

ricea Nov 27, 2018

MattiasBuelens Nov 27, 2018

domenic commented Nov 26, 2018

ricea commented Nov 27, 2018

domenic commented Nov 27, 2018

Add a brand check to pipeThrough() #966

Add a brand check to pipeThrough() #966

Conversation

ricea commented Nov 22, 2018 • edited by pr-preview bot Loading

domenic left a comment

Choose a reason for hiding this comment

domenic Nov 26, 2018

Choose a reason for hiding this comment

ricea Nov 27, 2018

Choose a reason for hiding this comment

domenic Nov 26, 2018

Choose a reason for hiding this comment

ricea Nov 27, 2018

Choose a reason for hiding this comment

MattiasBuelens Nov 27, 2018

Choose a reason for hiding this comment

domenic commented Nov 26, 2018

ricea commented Nov 27, 2018

domenic commented Nov 27, 2018

ricea commented Nov 22, 2018 •

edited by pr-preview bot

Loading