Allow aborting an ongoing pipe operation using AbortSignals #744

domenic · 2017-05-15T23:13:34Z

Closes #446.

Blocked on whatwg/dom#437 finishing up, but it was important to write this up as a proof of concept; it shows that the AbortSignal design is good for our needs.

There's a bit of a namespace collision in how streams has chosen writer.abort() to mean one thing which is a bit different from signal.abort() on a pipe. Maybe we should put a bit more about that in the developer-facing domintro boxes. But I don't think it's avoidable.

/cc @annevk

Preview | Diff

annevk · 2017-05-16T07:09:53Z

index.bs

@@ -700,6 +718,8 @@ ReadableStream(<var>underlyingSource</var> = {}, { <var>size</var>, <var>highWat
       an error _error_, which means to perform the following steps:
       1. Perform ! WritableStreamDefaultWriterRelease(_writer_).
       1. Perform ! ReadableStreamReaderGenericRelease(_reader_).
+       1. If _signal_ is not *undefined*, remove the above algorithm from _signal_'s <a for="AbortSignal">abort
+          algorithms</a>.


Maybe stuff the algorithm in a variable?

ricea · 2017-06-02T03:35:49Z

Text changes roughly lgtm. I'm not sure about the name "signal" for the option. I'm asking myself "would I remember that?" and I don't know the answer. Generally the AbortController / AbortSignal design feels like it has a lot of cognitive overhead but I haven't come up with anything better.

Compared to that, having two slightly different concepts called "abort" doesn't bother me.

annevk · 2017-06-22T16:44:51Z

The main reason we called it "signal" is that we might want to signal things other than abort too. And for Fetch at least the idea was those signals would all be on the same object.

domenic · 2017-07-05T11:58:20Z

@bakulf @tschneidereit any chance on implementer support from Firefox on this? Although this is less important than fetch() cancelation, it's also simpler, so we were thinking of using it as the first motivating case for AbortSignal in Blink at least.

I realize Firefox doesn't have writable streams / pipeTo yet, so it might be hard for you to judge fully, but I'm hopeful the spec is straightforward enough to get multi-implementer support anyway :)

annevk · 2017-07-05T12:53:36Z

Gecko implementation bug for abort things in general: https://bugzilla.mozilla.org/show_bug.cgi?id=1378342. I suspect we want this eventually, but it's highly likely we'll get fetch() done first.

tschneidereit · 2017-07-12T14:19:58Z

@domenic apologies for the late reply. I still haven't implemented pipeTo, but I do have WritableStream implemented (but not yet published, @bakulf and me are focusing on RS right now).

The changes here look quite straight-forward to me, though, so I'm happy to give support for them, with the condition that @annevk is happy with the implications for fetch.

annevk · 2017-07-13T07:10:42Z

As I understand it there won't be any implications for fetch() as it only uses ReadableStream. Maybe I'm missing something though.

tschneidereit · 2017-07-13T15:48:25Z

As I understand it there won't be any implications for fetch() as it only uses ReadableStream. Maybe I'm missing something though.

Sorry, that was confusing. What I meant was "as long as this satisfies use cases of specs that use streams, such as fetch, I'm happy to give support." That seems to be the case, but since I'm not familiar enough with the fetch spec to be quite certain on my own I wanted to flag this one more time.

domenic · 2017-07-13T19:35:57Z

Yeah, fetch cancelability should be pretty independent of this. Sounds like we're good, once the DOM PR is merged!

annevk · 2017-07-17T09:59:05Z

The DOM PR has merged.

domenic · 2017-07-17T18:20:31Z

Yep, just gotta write some tests.

ricea · 2018-09-06T12:48:56Z

Still lgtm. Do you want me to try adding it to the reference implementation and write some tests?

It looks like jsdom already has AbortController, so it should be pretty easy.

annevk · 2018-09-06T12:57:31Z

@ricea I strongly suspect that'd be much appreciated! (Rebased the PR.)

domenic · 2018-09-06T14:54:20Z

Yes please :). Sorry for letting this languish.

ricea · 2018-09-07T09:35:30Z

index.bs

 </div>

 <emu-alg>
  1. If ! IsReadableStream(*this*) is *false*, return <a>a promise rejected with</a> a *TypeError* exception.
  1. If ! IsWritableStream(_dest_) is *false*, return <a>a promise rejected with</a> a *TypeError* exception.
  1. Set _preventClose_ to ! ToBoolean(_preventClose_), set _preventAbort_ to ! ToBoolean(_preventAbort_), and set
     _preventCancel_ to ! ToBoolean(_preventCancel_).
+  1. If _signal_ is not *undefined*, and _signal_ is not an instance of the {{AbortSignal}} interface, return <a>a


I'm concerned that this condition may not be strong enough to guarantee we can execute the add algorithm.

Maybe we could use language similar to WebIDL, for example "and signal is not a platform object or does not implement AbortSignal, then ..."

The language above is similar to what we use elsewhere though (for "brand" checks), but arguably none of that is precise enough (there's an issue against IDL to define a [[PlatformBrand]] internal slot we could use to formally define these things).

Yeah, I think this is the right thing for now.

ricea · 2018-09-07T09:41:19Z

index.bs

+            WritableStreamAbort(_dest_, _error_) to _actions_.
+         1. If _preventCancel_ is *false*, <a for="set">append</a> the action of performing !
+            ReadableStreamCancel(*this*, _error_) to _actions_.
+         1. <a href="#rs-pipeTo-shutdown-with-action">Shutdown with an action</a> consisting of <a>waiting for all</a>


The waiting for all algorithm is problematic because it uses Promise.all(), which is very sensitive to changes to the global object.

We need to do something else, or change the definition of "waiting for all" in the promises guide.

I think the promises guide is known to be broken and needs to be uplifted into IDL. But nobody has done the work...

Right. Should we consider this blocked on fixing "waiting for all", or is it OK to continue using it in a "do what I mean, not what I say" fashion?

As far as I'm concerned we shouldn't block this on that, since there's presumably other dependencies that are similarly broken already? (I don't find it particularly great, but we seem to have no trouble adding dependencies on broken/unclear primitives all over the platform.)

ricea · 2018-09-27T13:18:27Z

I noticed a couple more issues while writing tests:

It doesn't work if the AbortSignal is already signalled when pipeTo() is called.
What happens when there are other shutdown conditions is not defined; eg. calling rs.pipeTo(ws, { signal }) where rs is already closed and signal is already signalled.

If we figure out the answer to the second question then I can solve the first.

ricea · 2018-10-01T12:58:57Z

From the point of view of the standard text (and probably the implementation), it's neater to check the signal at the beginning before looking at the state of the streams.

I think this behaviour is logical, so unless anyone objects, I'll go ahead and implement it.

annevk · 2018-10-01T13:11:24Z

That is what https://fetch.spec.whatwg.org/#dom-global-fetch does too, if I understand things correctly. We might need to document that pattern somehow more clearly so it's adopted by everyone consistently.

ricea · 2018-10-04T10:27:50Z

~~I probably should have noticed this before, but when pipeTo() is aborted, the promise is fulfilled with an AbortError. I think it should be rejected instead, to match the behaviour of fetch().~~

I misread it when I was implementing it. It does indeed reject.

Add tests for aborting ReadableStream.prototype.pipeTo using an AbortSignal. Tests correspond to PR whatwg/streams#744.

By bundling together the reference implementation using browserify, then evaluating it inside the wpt-runner window context, we fix all the cross-realm issues that have plagued us. This includes promise assimilation (which we had a workaround in place for), as well as Object.prototype identity (came up when testing async iterators) and the presence of DOMException and AbortSignal on the global (came up when testing #744).

Closes #446.

The reference implementation now supports the "signal" option to pipeTo(). It has diverged from the standard text, which will be updated separately. Tests are at web-platform-tests/wpt#13358.

The test for AbortError now also checks constructor.name, so fake that as well.

Also update the web-platform-tests pointer.

Previously, the reference implementation left _abortAlgorithm_ on _signal_ after pipeTo() exited. Remove it.

Reflect the fixes to the algorithms made in the reference implementation back into the standard. In particular, in order to correctly handle signals that are already aborted, _abortAlgorithm_ is defined earlier so that it can be called synchronously if necessary, and calls to WritableStreamAbort and ReadableStreamCancel are guarded by state checks.

ricea · 2018-10-25T12:15:36Z

I think this is ready to land. PTAL.

Add tests for aborting ReadableStream.prototype.pipeTo using an AbortSignal. Tests correspond to PR whatwg/streams#744.

domenic · 2018-11-07T22:20:46Z

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=902939. My understanding is other browsers don't have a pipeTo() implementation yet, so I didn't file bugs there, but let me know if folks want them.

…), a=testonly Automatic update from web-platform-tests[streams] Add tests for aborting pipeTo() Add tests for aborting ReadableStream.prototype.pipeTo using an AbortSignal. Tests correspond to PR whatwg/streams#744. -- wpt-commits: 349d41838068164e37a1cbd1c8c6ffeba4c72177 wpt-pr: 13358

…), a=testonly Automatic update from web-platform-tests[streams] Add tests for aborting pipeTo() Add tests for aborting ReadableStream.prototype.pipeTo using an AbortSignal. Tests correspond to PR whatwg/streams#744. -- wpt-commits: 349d41838068164e37a1cbd1c8c6ffeba4c72177 wpt-pr: 13358 UltraBlame original commit: d3eedf1cb5de6c78bdd8060836a53e1ab1e0574c

domenic added blocked do not merge yet Pull request must not be merged per rationale in comment piping labels May 15, 2017

annevk reviewed May 16, 2017

View reviewed changes

annevk removed the blocked label Jul 17, 2017

annevk force-pushed the abortsignals branch from 66ebc05 to 26a539a Compare September 6, 2018 13:11

ricea reviewed Sep 7, 2018

View reviewed changes

ricea mentioned this pull request Sep 21, 2018

Need newer version of jsdom for AbortController / AbortSignal domenic/wpt-runner#10

Closed

ricea added a commit to ricea/web-platform-tests that referenced this pull request Oct 4, 2018

[streams] Add tests for aborting pipeTo()

5a2e7b9

Add tests for aborting ReadableStream.prototype.pipeTo using an AbortSignal. Tests correspond to PR whatwg/streams#744.

ricea mentioned this pull request Oct 4, 2018

[streams] Add tests for aborting pipeTo() web-platform-tests/wpt#13358

Merged

domenic mentioned this pull request Oct 11, 2018

Meta: fix realm issues in reference implementation testing setup #957

Merged

domenic and others added 5 commits October 19, 2018 23:29

Allow aborting an ongoing pipe operation using AbortSignals

ded08c3

Closes #446.

Implement pipeTo() abort in the ref impl

f25c7e0

The reference implementation now supports the "signal" option to pipeTo(). It has diverged from the standard text, which will be updated separately. Tests are at web-platform-tests/wpt#13358.

Update web-platform-tests commit

82eef16

Fake the constructor name on fake AbortError

e2965a8

The test for AbortError now also checks constructor.name, so fake that as well.

Remove workarounds for missing symbols

31079ac

Also update the web-platform-tests pointer.

ricea force-pushed the abortsignals branch from c89e627 to 31079ac Compare October 19, 2018 14:36

ricea added 5 commits October 19, 2018 23:58

Throw AbortError correctly

b2239ed

Update web-platform-tests submodule pointer

50e0ebd

Correctly remove abortAlgorithm on exit from pipeTo()

4fa04fe

Previously, the reference implementation left _abortAlgorithm_ on _signal_ after pipeTo() exited. Remove it.

Update web-platform-tests pointer

a0247dd

ricea removed the do not merge yet Pull request must not be merged per rationale in comment label Oct 25, 2018

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 7, 2018

[streams] Add tests for aborting pipeTo()

349d418

Add tests for aborting ReadableStream.prototype.pipeTo using an AbortSignal. Tests correspond to PR whatwg/streams#744.

Update WPT submodule pointer to master

8e0aaed

domenic merged commit 46c3b89 into master Nov 7, 2018

domenic deleted the abortsignals branch November 7, 2018 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow aborting an ongoing pipe operation using AbortSignals #744

Allow aborting an ongoing pipe operation using AbortSignals #744

domenic commented May 15, 2017 •

edited by pr-preview bot

Loading

annevk May 16, 2017

ricea commented Jun 2, 2017

annevk commented Jun 22, 2017

domenic commented Jul 5, 2017

annevk commented Jul 5, 2017

tschneidereit commented Jul 12, 2017

annevk commented Jul 13, 2017

tschneidereit commented Jul 13, 2017

domenic commented Jul 13, 2017

annevk commented Jul 17, 2017

domenic commented Jul 17, 2017

ricea commented Sep 6, 2018

annevk commented Sep 6, 2018 •

edited

Loading

domenic commented Sep 6, 2018

ricea Sep 7, 2018

annevk Sep 7, 2018

domenic Sep 7, 2018

ricea Sep 7, 2018

annevk Sep 7, 2018

domenic Sep 7, 2018

annevk Sep 7, 2018

ricea commented Sep 27, 2018

ricea commented Oct 1, 2018

annevk commented Oct 1, 2018

ricea commented Oct 4, 2018 •

edited

Loading

ricea commented Oct 25, 2018

domenic commented Nov 7, 2018

Allow aborting an ongoing pipe operation using AbortSignals #744

Allow aborting an ongoing pipe operation using AbortSignals #744

Conversation

domenic commented May 15, 2017 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

ricea commented Jun 2, 2017

annevk commented Jun 22, 2017

domenic commented Jul 5, 2017

annevk commented Jul 5, 2017

tschneidereit commented Jul 12, 2017

annevk commented Jul 13, 2017

tschneidereit commented Jul 13, 2017

domenic commented Jul 13, 2017

annevk commented Jul 17, 2017

domenic commented Jul 17, 2017

ricea commented Sep 6, 2018

annevk commented Sep 6, 2018 • edited Loading

domenic commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea commented Sep 27, 2018

ricea commented Oct 1, 2018

annevk commented Oct 1, 2018

ricea commented Oct 4, 2018 • edited Loading

ricea commented Oct 25, 2018

domenic commented Nov 7, 2018

domenic commented May 15, 2017 •

edited by pr-preview bot

Loading

annevk commented Sep 6, 2018 •

edited

Loading

ricea commented Oct 4, 2018 •

edited

Loading