Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define a 'CancelationController' and 'CancelationSignal' interface. #434

Closed
wants to merge 10 commits into from
Closed

Define a 'CancelationController' and 'CancelationSignal' interface. #434

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3f57df3
Define a 'PromiseController' and 'PromiseSignal' interface.
mikewest Apr 5, 2017
61c6e72
fixup 'abort steps'
mikewest Apr 6, 2017
9282bf3
fixup AbortError and whitespace.
mikewest Apr 6, 2017
8681c81
fixup abort steps before event.
mikewest Apr 6, 2017
edce169
fixup rename to 'CancellationController'. Initial feedback.
mikewest Apr 6, 2017
f64a92f
fixup matching DOM's strange spelling of 'cancelled'
mikewest Apr 7, 2017
36028dd
fixup SameObject
mikewest Apr 7, 2017
b744aa7
fixup ordered set of callbacks.
mikewest Apr 7, 2017
28b01bc
fixup disambiguate canceled flag.
mikewest Apr 7, 2017
4273de9
fixup 'CancelationError' is now a 'DOMException'.
mikewest Apr 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions dom.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,6 +1419,98 @@ that gave folks all the wrong ideas. <a>Events</a> do not represent or cause act
can only be used to influence an ongoing one.


<h3 id=controlling-promises>Controlling {{Promise}}s</h3>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably doesn't belong under the "Events" section. I'm not sure if it belongs in DOM at all, but I don't know of a better place really, so who knows... new WHATWG spec time?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is using IDL fine?
  2. How big can this possibly get?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDL should be fine. I can't imagine this getting much bigger; new spec was mostly in jest.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is to say, if this remains fairly small long term, I don't like the idea of it being on its own. In DOM seems somewhat reasonable given that events are signals of sorts too, but it's pushing things a bit too of course.

And if this needs to be a non-IDL thing, perhaps Streams is a good place? It's also fits somewhat and Fetch has a dependency on that anyway, as will many other things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it has a dependency on HTML via EventHandler, somewhat amusingly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to y'all about where you'd like this to live. I have no strong opinion, so long as it lives somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually isn't really specific to promises. For example it could be used to cancel a stream:

const readableStream = getStream({ signal });

// ... later ...
signal.abort(); // basically the same as readableStream.cancel()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CancellationController? That seems reasonable as a base class for FetchController if @jakearchibald needs such a thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, it's not clear to me why you'd use this model for streams when they have a built in cancellation mechanism. Do you think it's worth changing the streams API in line with what we have to do here for Promises?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea would be to allow people to pass in the signal to many APIs, streams being just one. So it's OK if an API has both a dedicated cancel mechanism and also accepts the platform-generic one, IMO.


<p>Though {{Promise}} objects don't have any built-in cancellation mechanism, many
{{Promise}}-vending APIs require cancellation semantics. {{PromiseController}} is meant to support
these requirements by providing an {{PromiseController/abort()}} method that toggles the state of
a corresponding {{PromiseSignal}} object. The API which wishes to support cancelation can accept
such a {{PromiseSignal}}, and use its state to determine how (not) to proceed.

<div class=note>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<div class=example id=...> not <div class=note>

For example, a hypothetical <code>DoAmazingness({ ... })</code> method could accept a
{{PromiseSignal}} object in order to support cancellation as follows:

<pre class=lang-javascript>
const controller = new PromiseController();
const signal = controller.signal;
DoAmazingness({ ..., signal }).then(result => ...);

...

controller.abort();
</pre>

APIs that require more granular control could extend both {{PromiseController}} and
{{PromiseSignal}} according to their needs.
</div>

<h4 id=interface-promisecontroller>Interface {{PromiseController}}</h3>

<pre class="idl">
[Constructor(), Exposed=(Window,Worker)]
interface PromiseController {
readonly attribute PromiseSignal signal;

void abort();
};
</pre>
<dl class=domintro>
<dt><code><var>controller</var> = new <a constructor lt="PromiseController()">PromiseController</a>()</code>
<dd>Returns a new <var>controller</var> whose {{PromiseController/signal}} attribute value is set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove "attribute value" (in general mentioning "attributes" in dev-facing docs is just confusing).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

to a newly created {{PromiseSignal}} object.

<dt><code><var>controller</var> . </code>{{PromiseController/signal}}
<dd>Returns the {{PromiseSignal}} object associated with this object.

<dt><code><var>controller</var> . </code>{{PromiseController/abort()}}
<dd>Invoking this method will set this object's {{PromiseSignal}}'s [=PromiseSignal/aborted flag=],
thereby signaling to any observers that the associated activity should be aborted.
</dl>

The <dfn attribute for=PromiseController>signal</dfn> attribute must return the value to which it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs <code>

was initialized. When a {{PromiseController}} is created, the attribute must be initialized to a
newly created {{PromiseSignal}} object.

The <dfn method for=PromiseController><code>abort()</code></dfn>, when invoked, must run these
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing "method"

steps:

<ol>
<li>Let <var>signal</var> be this object's {{PromiseController/signal}}.
<li>Set <var>signal</var>'s [=PromiseSignal/aborted flag=].
<li>[=Fire an event=] named <code>abort</code> at <var>signal</var>.
<li>Run <var>signal</var>'s [=PromiseSignal/abort steps=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we probably want the event to dispatch after the abort steps have run as the event listeners can cause side effects the abort steps might not anticipate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inverted in 8681c81.

</ol>

<h4 id=interface-cancellationcontroller>Interface {{PromiseSignal}}</h4>

<pre class="idl">
[Exposed=(Window,Worker)]
interface PromiseSignal : EventTarget {
readonly attribute boolean aborted;

attribute EventHandler onabort;
};
</pre>
<dl class=domintro>
<dt><code><var>signal</var> . </code>{{PromiseSignal/aborted}}
<dd>Returns true if this {{PromiseSignal}} has been aborted, and false otherwise.
</dl>

Each {{PromiseSignal}} has an <dfn for=PromiseSignal>aborted flag</dfn> which is unset unless
otherwise specified.

Each {{PromiseSignal}} has an <dfn for=PromiseSignal>abort steps</dfn> algorithm which is
executed when its [=PromiseSignal/aborted flag=] is set. Unless otherwise specified, this
algorithm is a no-op.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so how would this be used? It may need an example. Is the idea that e.g. fetch() would accept a signal, then mutate its abort steps to abort the fetch? What if you pass the same signal in to multiple fetches? Maybe it should be a list of abort steps, so that consumers append to the list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that Fetch would subclass these and define custom steps there. Having the steps be part of the base class helps in that you don't have to mutate most other things. How Fetch handles multiple fetches and such could be done in such a setup with a single set of abort steps.

Now, I don't know for sure that Fetch would subclass these. @jakearchibald should probably weigh in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see. Fetch would subclass, but streams (and possibly whatever @mikewest wants this for originally) would not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe you're right that we should design this more so that it even works for all scenarios when not subclassed. I do think that whatever this get passed to needs to be able to hook into it, since we don't really have specification-only event listeners (I think that would be a bigger thing to add).


<p class="note no-backref">The [=PromiseSignal/abort steps=] algorithm enables APIs with more
complex requirements to react in a reasonable way to {{PromiseController/abort()}}. For example,
a given API's [=PromiseSignal/aborted flag=] may need to be propagated to a cross-thread
environment (like a Service Worker).

The <dfn attribute for=PromiseSignal>aborted</dfn> attribute's getter must return true if the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs <code>

object's [=PromiseSignal/aborted flag=] is set, and false otherwise.

<h2 id=nodes>Nodes</h2>

Expand Down