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.

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

Automatic cancellation support for Promises #28

Open
martinheidegger opened this issue Apr 3, 2020 · 29 comments
Open

Automatic cancellation support for Promises #28

martinheidegger opened this issue Apr 3, 2020 · 29 comments

Comments

@martinheidegger
Copy link

I have been working with abort signals and the biggest issue in practically using it was to make sure that all the pieces of code i wrote pass-on the signals properly. Missing on passing meant that a whole part of my code could be cut-off and it is rather stressful to test all the edge cases.

This made me think of an extended API that could make the process a lot easier: Assuming we had a .cancel(token: CancellationToken): this and a means to name a given token for the Promise it could be possible to create nice zones for promises:

// the @-token is to declare a "token" variable for the foo scope.
async@token function foo () {
  token.throwIfCancellationRequested()
  // Note that the signal is not specifically passed on here
  await bar()
}

const source = new CancellationTokenSource()
await (foo().cancel(source.token))
await@source.token foo() // syntax sugar for the previous operation

await foo() // could automatically connect to the process closing signal of node.

async function bar () {
  // Even without specifying the signal, the 
  await baz()
}

async@myvarname function baz () {
  // Using a non-conflicting variable name
  myvarname.throwIfCancellationRequested()
}

This could be built in with Promise.race where the promises after the first promise get automatically cancelled.

@ljharb
Copy link
Member

ljharb commented Apr 3, 2020

This seems to conflate the two distinct uses for "cancellation" - "registering disinterest", ie, an unthen method; and "aborting the work".

An individual Promise is supposed to confer no privileges except reacting to the final fulfillment - as such, a cancel method on Promises would imo be inappropriate, except perhaps as an unthen (ie, promise.then(f); promise.unthen(f);)

@martinheidegger
Copy link
Author

Yes, there is an ambiguity in the intent. Is it just disinterest or an active cancellation. Though I don't see how this distinction has an impact for the implementor. The implementor just needs to make sure that the work is stopped in either case.

How else would this distinction have impact?

As for the "cancel" naming: yes this should be called differently. I didn't find a better name but I generally meant to illustrate that instead of the syntax sugar this could also be a method. unthen seems better.

@ljharb
Copy link
Member

ljharb commented Apr 4, 2020

The only thing a method on a Promise should do imo is "unthen" - which should not notify the creator of the Promise, for example.

@benjamingr
Copy link

@ljharb for what it's worth that issue is pretty simple and can be addressed:

  • Async functions perform an action, so cancellation and aborting them makes sense.
  • Promises are just a result + time, so disinterest makes sense but actually aborting actions makes less sense since there is now a notion of a "promise's underlying value obtaining mechanism". I agree that breaks the abstraction.
  • Async functions return a Promise and await promises (well, thenables), so it is impossible to return extra information about cancelling an action on them without violating the "just a value" semantic of promises.

If these async functions returned something that is not a Promise but is either a subclass (worse) or a thenable but not a subclass (better IMO because LSP) we wouldn't have an issue:

Specifically in the context of this proposal:

async function foo() { } // calling this returns a Promise
async@token function foo () {} // calling this returns a CancellableTask

With a CancellableTask not having the same "promise" expectation, and awaiting a CancellableTask in a regular async function making it fall-back to a regular promise (just because it's a thenable.

No promise functions would be "cancellation aware", no user expectations regarding promises would change and this would strictly be an extension.

Note: I am not saying TC39 should do this, that this is how cancellation should proceed or that I think this is a Good Idea™. I am also not saying that the fact a Promise should contain only value state is fact (it's an opinion I sometimes hold too) nor that it's consensus (it is however a valid concern IMO).

I am only trying to explain how we can reconcile cancellation with that constraint with the discussion :]

As a side note,

@ljharb
Copy link
Member

ljharb commented Apr 4, 2020

I agree that syntax to make an otherwise async function return a cancellable Task instead of a Promise would address the issues i mentioned in this thread (not endorsing that idea necessarily either).

@Jamesernator
Copy link

While I agree with:

An individual Promise is supposed to confer no privileges except reacting to the final fulfillment

I disagree that implies they shouldn't be cancelable, in fact given Promise is the primary mechanism for asynchronous operations in the spec, if cancellation is added then Promises should be cancelable.

However I don't think someone who has reference to a Promise should be able to cancel it. I think cancel() should be something controlled by whoever creates the Promise.

For example in the Promise constructor:

function delay(time, cancelToken=null) {
  return new Promise((resolve, reject, cancel) => {
    const timeout = setTimeout(resolve, time);
    cancelToken?.register(() => clearTimeout(timeout));
  });
}

This is basically the status quo with AbortSignal, however I think it'd be good if cancellation signals automatically propagated through the call stack.

For example through the new metaproperty like function.cancelToken:

function delay(time) {
  const cancelToken = function.cancelToken;
  return new Promise((resolve, reject, cancel) => {
    const timeout = setTimeout(resolve, time);
    cancelToken?.register(() => clearTimeout(timeout));
  });
}

async function dobuleTimeout() {
  await delay(1000);
  await delay(1000);
}

const { cancelToken, cancel } = CancelToken.create();
const doubletTimeoutPromise = doubleTimeout.start(cancelToken);

// Some time later, this can abort whichever delay is currently happening
cancel();

@getify
Copy link

getify commented Apr 5, 2020

If you already hold the reject(..) capability for a promise, why do you need a separate cancel(..) capability?

Are you implying that consumers of a promise need to be notified of a promise cancellation distinct from a rejection, and thus we need to extend promises with a third final-state and accept a third callback to .then(..) (and/or expose a canceled(..) convenience method like catch(..))?

I've done an awful lot of cancellation (via AbortController tokens) propagated through as promise rejections over the last 2-3 years (via my CAF lib), and I've been quite happy to signal this cancellation via specific error classes/messages (as the promise-rejection reason) and then try..catch them as needed up the call-stack.

This has been totally sufficient so far; I haven't come across any need to split a promise into a third final-state. Am I missing something other than a light semantic flourish driving that?

Furthermore, if promise-cancellation didn't map to synchronous-flow-control as a catchable exception, then try..catch itself would need to be extended to declaratively handle this third state to preserve the strong trend of async..await style sync-async flow control (over promise chains).

@Jamesernator
Copy link

If you already hold the reject(..) capability for a promise, why do you need a separate cancel(..) capability?

I'm ambivalent as long as there's a concrete way of detecting a cancellation reliably from other errors. Personally I'd be a fan of just seeing CancellationError and the DOMException for abort just inheriting from it.

@getify
Copy link

getify commented Apr 5, 2020

Magical propagation of cancel tokens through the call-stack is cool (function metaproperty, etc). But I don't see why promises themselves need to be changed in any way to support cancellation. A promise rejection can carry whatever sort of cancellation "signal" you want.

@Jamesernator
Copy link

Jamesernator commented Apr 5, 2020

But I don't see why promises themselves need to be changed in any way to support cancellation.

Mostly I was meaning that Promises themselves should contain the cancellation (whether through another state or not I don't really care), rather than some other thing like the proposed Task in another comment.

@ljharb
Copy link
Member

ljharb commented Apr 5, 2020

@Jamesernator i agree only the creator of the promise should initially have the ability to cancel anything; i'm saying a Promise instance method wouldn't achieve that.

@benjamingr
Copy link

@Jamesernator I think there is little point to arguing for promise cancellation with the Promise type when we can reconcile the concern by not putting it on the promise type.

If you look at my comment #28 (comment) I think we can reconcile these concerns so everyone wins.


@getify

This has been totally sufficient so far; I haven't come across any need to split a promise into a third final-state. Am I missing something other than a light semantic flourish driving that?

That's a valid concern. Mostly, using rejection for cancellation semantics can be dangerous at times. We used to call this the "don't wake me up" criterion" in the old proposal.

Let's say I have an async action:

async function cancelMe(token) {
  try {
     await someAsyncAction(token);
  } catch (err) {
     instrumentation.track(err);
  }
}

If the token is cancelled, the } catch { is hit and the instrumentation is updated. Now every single catch block needs to guard against cancellation exceptions - and worse: catch clauses with await might cause finally blocks to not run.

Is this terrible or worse than no cancellation? Not really, it would still be an improvement.

Did I suffer using languages that picked this design? Yes, it actually did in fact wake me up at night once or twice when Cancellation errors made their way through the instrumentation.

If we look a bit at other languages - C# does reject semantics - that has multiple issues (like triggering catch blocks) but is better than not doing anything.

F# does what Martin's proposal suggests - it checks cancellation tokens automatically at every await (well, do!) - if it was cancelled a CancellationException is thrown (so a bit like your proposal). I encourage you to check out F# cancellation to see how it behaves :]

Several places in JavaScript (namely generators) already have third state.

To sum up my opinion:

  • Third state is probably a good idea. It is useful so that non-exceptional cases don't go through exceptional flows and end up triggering instrumentation.
  • Not having third state is not terrible and is still probably better than no cancellation. We have examples of languages with similar cancellation semantics without this proposal (like C#) and with this proposal (like F#).
  • Whether or not we want to have third-state is probably a trade-off between correctness and simplicity. Having fewer states is easier while having states that incorrectly map the user expectation is more "correct". I don't think there is a a right answer here.
  • If we look at historical data when tools had a choice they switched from reject semantics to third state (I can share some insights from bluebird if that helps).
  • I don't think anyone is pursuing third-state anyway at this point - so probably better to focus on two-state less-correct-but-simpler cancellation in the context of Martin's proposal.

@martinheidegger
Copy link
Author

martinheidegger commented Apr 5, 2020

Okay, this discussion quickly swung in a lot of directions. First off big 😍 for the lots of responses here. In an attempt that probably leaves me sweating tonight let's see if I can address all the issues in an order that could in the end somehow make sense. 😅

(I am sorry for the length. I spent all day writing on it, don't know how to shorten it further and am running out of time. 😢 )

if these async functions returned something that is not a Promise but is either a subclass (worse) or a thenable but not a subclass (better IMO because LSP) we wouldn't have an issue:

Thank you @benjamingr for coming up with this example:

async function foo() { } // calling this returns a Promise
async@token function foo () {} // calling this returns a CancellableTask

this could be very agreeable to implement, and as a short note I think we would run into the question to do either type-check the result of await bar() to see if the token can be passed-on or not or we need to specify this with every await@token call manually.

However, what makes this approach very problematic to me is that all sub-operations of the given promise may never become cancel-able tasks as @Jamesernator illustrated nicely in the "delay example".

I think there is little point to arguing for promise cancellation with the Promise type when we can reconcile the concern by not putting it on the promise type.

Passing a token to a async function or Promise replaces an API with an convention. The convention of passing-in the token is prone to ...

  • ... manual errors: people forgetting to pass down a token, pass the wrong token or name it badly
  • ... problem invisibility: as it is optional to pass a signal, not passing it is a valid case
  • ... purposeful ignorance: some users may not need the tokens but underlying api's support it

Most of my proposal was supposed to focus on preventing to manually passing down that token to all sub-operations. @getify notes:

done an awful lot of cancellation (...) propagated through as promise rejections over the last 2-3 years...

but propagating those rejections is only one direction, passing the signals down the chain is the harder issue to me. I like to avoid two different return types for await and await@token and because of that am trying to see if alternative routes can be better.

With this in mind, looking at the alternative syntax proposed:

async function dobuleTimeout() {
  await delay(1000);
  await delay(1000);
}

here dobuleTimeout does not specify the cancellation-control of the two promises created. We are assuming that there is a pass-through happening here. I also assume that the await is just coincidental. This should be functional(, right?):

async function dobuleTimeout() {
   const p = delay(1000)
   await p
   await delay(1000)
}

... and that would mean that every call inside of dobuleTimeout would pass the cancellation-context to every subsequent call! And, even more troublesome, the question arises: Is this also true for regular functions?

function dobuleTimeout() {
   return delay(1000).then(() => delay(1000))
}

Does it pass on the context similarly? Or do I need to manually operate here?

function dobuleTimeout() {
   const ctx = function.context
   return delay.start(ctx, 1000).then(() => delay.start(ctx, 1000))
}

My assumption again is that it should also work for non-async functions as well. As such, we are talking here about a "general function context" or "domain" which could also be implemented as a general-purpose context like dobuleTimeout.start({[cancelSymbol]: token}).

This might have a slew of horrible side-effects. I tried to avoid this a scary concept as these side-effects are generally hard to identify (debugging hell?!) and I am not sure if its worth the risk.


Another (smaller) thing I wanted to avoid is this:

cancelToken?.register(

as this means that there might be the possibility for the .cancelToken to not exist, which could result in (unnecessary) null errors. Using an optional parameter could make sure that those tokens are only created if necessary by the implementer:

async@token function foo() {
   token.register(...)
}

async function bar () {
   // Doesn't use the token, token is not created.
}

Having it be a separate async syntax also highlights that this only affecting the async/Promise system and does not interact with general function calls. Regular Promise instantiations would have it as third argument:

new Promise((resolve, reject, token) => {})

this could avoid the aforementioned debugging hell by moving the magic from a function call to the await operation with a utilizing the token of every promise. We would only need to "link" those tokens up to create the same conceptual domain.

If you already hold the reject(..) capability for a promise, why do you need a separate cancel(..) capability?

Sorry for the confusion, in my initial proposal I was trying to suggest that the inappropriately named .cancel method would do following that has nothing to do with reject:

function cancel(token) {
   bindTokens(token, this.token)
   return this
}

where bindTokens would connect the cancel call to the promises token:

const p = delay(1000)
const { cancel, token } = createToken()
bindTokens(token, p.token) // p.token and token form a graph, cancel will also inform the promise's token - this is a domain

Promises would need to pass it on manually, to keep the magic small.

new Promise((resolve, reject, token) => {
  const otherPromise = doAsyncThing()
  bindTokens(token, otherPromise.token) // very rare case
  otherPromise.then(resolve, reject)
})
Note on naming

I chose bindTokens as it is quick to write and easy to read. But it should probably be something like CancelToken.connectTokens(token, p.token).

Also bindTokens is a functional implementation. This could be a member operation of cancelToken, e.g. token.addChild(p.token).

I went with a placeholder .token variable name here. Probably something like .cancelToken or even [CancelToken.symbol] would be better.

@Jamesernator

I think cancel() should be something controlled by whoever creates the Promise.

Honestly, I hadn't thought of this. Thank you for pointing it out!

To make sure that the creator stays in control, bindTokens could be a one-time operation; throwing an error if that token was already bound.

const p = delay(1000)
const { cancel, token } = createToken()
bindTokens(token, p.token)
bindTokens(createToken().token, p.token) // throws CancelTokenBindError() - or something like that 

Note: An additional function bindTokenIfUnbound could be neccessary, as this is how await is supposed to behave.

@ljharb

The only thing a method on a Promise should do imo is "unthen" - which should not notify the creator of the Promise, for example.

I didn't understand the purpose of unthen in the first comment. This has cleared it up quite a bit. Thanks!

If I understand you right then unthen means to achieve that the system could clean up memory and drop certain operations as they become unnecessary (no one awaiting the response). an operation that an operation like this does have benefits, but I was solely concerned with cancel here. I think with the one-time binding it should be cleared that I only meant the cancel case and not the un-registration.


Are you implying that consumers of a promise need to be notified of a promise cancellation distinct from a rejection, and thus we need to extend promises with a third final-state and accept a third callback...

My naming might have caused this confusion: I am sorry 😓 . The third state should imo. be a separate discussion.

@Jamesernator
Copy link

Jamesernator commented Apr 5, 2020

here dobuleTimeout does not specify the cancellation-control of the two promises created. We are assuming that there is a pass-through happening here. I also assume that the await is just coincidental. This should be functional(, right?):
Does it pass on the context similarly? Or do I need to manually operate here?

Yes, it would simply pass it down the stack, everything on the call stack gets the cancelToken.

As such, we are talking here about a "general function context" or "domain" which could also be implemented as a general-purpose context like dobuleTimeout.start({[cancelSymbol]: token}).

This might have a slew of horrible side-effects. I tried to avoid this a scary concept as these side-effects are generally hard to identify (debugging hell?!) and I am not sure if its worth the risk.

The idea of this is to only propagate cancellation, as such to avoid people abusing it as a general purpose context function.cancelToken should be a different object in each frame on the call stack. Which is to say, they cannot be used to share data:

function b() {
  console.log(function.cancelToken.foo); // undefined
}

function a() {
  const { cancelToken } = CancelToken.create();
  cancelToken.foo = 'foobar';
  b.start(cancelToken);
}

@martinheidegger
Copy link
Author

Which is to say, they cannot be used to share data.

Thank you for adding this! It is a neat solution and makes that approach feel significantly better! But it doesn't resolve my fears of a slippery slope here. Once people see that this way of passing down control for cancelTokens, other side-effects might be added using the same route which might be less considerate.

@benjamingr
Copy link

@martinheidegger

to see if the token can be passed-on or not or we need to specify this with every await@token call manually.

However, what makes this approach very problematic to me is that all sub-operations of the given promise may never become cancel-able tasks as @Jamesernator illustrated nicely in the "delay example".

Thanks for the detailed response :]

To clarify, the semantics I envisioned reconciling this proposal with the expectation of promises not carrying state about their originating operation is:

  • Cancellation propagates automatically in AsyncFunctions. I'll go over an examples and how it works below.
  • Cancellation does not work with regular promises at all, it is impossible to "cancel a regular promise". In addition once you go through a promise combinator the result of that is not cancellable unless you explicitly went through CancellableTask.
  • You never have to write .register or pass tokens explicitly.
// here doubleTimeout is an asyncFunction with a token, so it returns a CancellableTask
// and not a regular promise
async@token function doubleTimeout() {
  // if delay returns a CancellableTask, it is cancelled automatically when doubleTimeout is and 
 // invoking it automatically passes the token inside.
  await delay(1000);
  // if delay returns a regular promise, doubleTimeout is exited after cancelling but nothing 
  // happens to the promise itself, except perhaps `unthen` if we choose to do that.
  await delay(1000);
}

Note that in this sort of design the token is determined based on being called in an async function with a token and not by awaiting.

In:

function dobuleTimeout() {
   return delay(1000).then(() => delay(1000))
}

I would recommend not supporting cancellation propagation of promises at all and sticking to async functions - but in the above case since no token is passed to the function dobuleTimeout is only "as cancellable" as delay inside it and is not "cancellation aware" (it's just a regular function - that didn't change). In the above case the question is "does chaining thens preserve cancellation/token and does that token pass to delay".

I would not pass the token to delay above (eventhough technically it is possible since we know what the token is and we can chain them automatically) because I think that complicates things significantly when dealing with assimilation and that's not the "interesting" case for most people who use async functions and need to cancel.

@RangerMauve
Copy link

Just throwing this out there, but it'd be cool if we could pass a token as the third argument to .then which would enable the promise to listen to it for cancellation if it could / do nothing if it can't.

It'd be cool if thennables that support this third parameter could then pass it on to any calls to .then that they make.

So when you call an async function, you can pass the token to .then or automatically do it if you're an async function that got a token passed to it already.

This approach probably doesn't work too well for sync cancellation though since there could be a bit between calling the function and invoking .then where no token is present for the function to pass to it's own invocations of .then. 🤷

Maybe there could be a token created internally and it's listening on any subsequent calls to .then?

Oh jeeze, if two places are listening on a promise I'd imagine you wouldn't want the cancellation of one to affect the other. I guess that's why unthening is the safest option. 🤔

@dead-claudia
Copy link

@ljharb How is disinterest anything other than cancelling a request to retrieve a value? You're already effectively scheduling a job to retrieve that value by using .then (which is a lot of ceremony for just retrieving a value IMHO - it's not a simple .getValue() method call, and the spec internals make that clear in painstaking detail), and cancellation could be arguably viewed as cancelling that request (or cancelling the reaction job, in ES spec speak).

@ljharb
Copy link
Member

ljharb commented Sep 6, 2020

@isiahmeadows disinterest is "go ahead and finish the work, i don't care, don't notify me when you're done". Cancelling is "stop the work!".

A Promise represents a placeholder for a future value for work that has already begun.

@getify
Copy link

getify commented Sep 6, 2020

I wish there was a way to determine how many usages of userland "cancellation" semantics on promises are about affirmative cancellation vs just disinterest. My intuition (based mostly just on some anecdotal observations in different code bases) is that the later is greater than the former. But it seems like if there was any way to gather evidence like that, it would help inform the design decisions here. If the majority use-case is actually dis-interest, unthen(..) might be enough.

@ljharb
Copy link
Member

ljharb commented Sep 6, 2020

@getify I agree with your intuition, and "disinterest" is something that wouldn't violate any mental models around Promises - but "abort the work" is something that only the creator of the Promise must be able to control (or, ofc, explicitly delegate).

The trick with .unthen is that const b = a.then(c) doesn't actually mutate a, so a.unthen(c) wouldn't make any sense. b is a brand new promise, so what would b.unthen(c) do? Would it just make sure b never settles? What about if someone had done const e = b.then(d); b.unthen(c) - would e also never settle? or would e silently "become" a.then(d)?

@dead-claudia
Copy link

@ljharb I didn't dispute that. But cancelling your request of that data is showing disinterest to it. I feel you're looking higher level than I am - I'm looking at what .then does, not what the promise itself does. And I agree with you it shouldn't affect promise resolution - that's not what I'm talking about.

The trick with .unthen is that const b = a.then(c) doesn't actually mutate a, so a.unthen(c) wouldn't make any sense. b is a brand new promise, so what would b.unthen(c) do? Would it just make sure b never settles? What about if someone had done const e = b.then(d); b.unthen(c) - would e also never settle? or would e silently "become" a.then(d)?

a.then(func) does mutate a if it's pending - see step 9.a/9.b of PerformPromiseThen. That's how the value is propagated. And here's the semantics I'd expect of both cancellation and of a conceptual .unthen method:

  • Cancelling a.then(c), if c was not yet invoked, would result in b never resolving, as the promise reaction that would eventually resolve b would never end up executing.
  • Cancelling a.then(c), if c was already invoked, would observably do nothing as the job to cancel was already executed and b either is already being resolved or was already resolved.
  • Cancelling b.then(d) would not do anything to a.then(c), as a and b are different promises, and cancellation would only mutate the immediate promise itself.
  • When a.then(c) is cancelled and c was not yet invoked, c can be dereferenced as well as everything b references, avoiding a whole class of associated memory leaks with it.

And BTW, using cancellation would make useless things like b.unthen(c) and a.unthen(d) unrepresentable, so that's a question that wouldn't need answered. It also avoids the question of what to do in cases like this, as it doesn't have any knowledge of the function to be invoked (or even the resulting promise, for that matter):

const p1 = a.then(foo)
const p2 = a.then(foo)
a.unthen(foo) // ???

@dead-claudia
Copy link

@getify

I wish there was a way to determine how many usages of userland "cancellation" semantics on promises are about affirmative cancellation vs just disinterest.

Maybe a poll? Also, it might be worth doing a couple things special in that poll:

  1. Make it clear you're not talking about the new Promise(...) stuff and similar, only p.then(f).
  2. Allow people to specify both disinterest and cancellation independently of each other, not just leaving it as a binary option (as some, like me, would likely consider the two to be synonymous and would respond accordingly).

@ljharb
Copy link
Member

ljharb commented Sep 6, 2020

It doesn't mutate it in a way that's observable. You can't tell, in your example, whether a got a then callback or not (for the purposes of this discussion, unhandled rejection hooks don't exist, since engines aren't required to implement them)

@dead-claudia
Copy link

dead-claudia commented Sep 6, 2020

@ljharb You can observe such mutation asynchronously very easily by taking advantage of callback order:

let handlerRemoved = true
let child = a.then(() => {
    handlerRemoved = false
})
runWithDifferentCancelToken(() => {
    a.then(() => {
        console.log(handlerRemoved)
    })
})

While it can't be observed synchronously, cancellation isn't just for things that start synchronously - it's just as useful for aborting multi-step asynchronous tasks that otherwise run in the background.

Edit: wording

@ljharb
Copy link
Member

ljharb commented Sep 6, 2020

Let me rephrase :-) short of the promise settling, you can't observe it.

The word "cancel" continues to conflate the two separate concepts - whether it's useful to abort multi-step async tasks isn't in dispute, but whether it's feasible to allow that in the Promise security model is.

@benjamingr
Copy link

I can ask Node to run a survey though we just ran one for unhandledRejection.

Anyway I didn't realize abort semantics were even on the table. I thought that disinterest semantics were consensus as abort semantics are dangerous.

That's why bluebird switched to disinterest like 5 years ago.

@ljharb
Copy link
Member

ljharb commented Sep 6, 2020

I suspect you're right. Unfortunately it's very difficult to use the word "cancel" and not create confusion as to which semantics are being described :-/

@dead-claudia
Copy link

@benjamingr

Anyway I didn't realize abort semantics were even on the table. I thought that disinterest semantics were consensus as abort semantics are dangerous.

Yeah, I was also confused when that was assumed by @ljharb, and that's probably what led to the communication disconnect between us. My bad - I should've been way clearer in that I was specifically not referring to cancelling the original promise, only .then scheduling its reaction job for removal upon cancellation.

@ljharb

The word "cancel" continues to conflate the two separate concepts - whether it's useful to abort multi-step async tasks isn't in dispute, but whether it's feasible to allow that in the Promise security model is.

If it helps, here's what I'm thinking:

function delay(ms) {
	return new Promise(resolve => {
		const timer = setTimeout(() => resolve(), ms)

		// In this case, we're cancelling the timer and leaving the promise resolved,
		// something already well within scope of the proposal
		onCancel(() => clearTimeout(timer))
	})
}

// Think of this as like a spec implementation.
function PerformPromiseThen(promise, onFulfilled, onRejected, resultCapability = undefined) {
	let promiseData = getInternalData(promise)

	if (promiseData.state === "pending") {
		let fulfillReaction = new PromiseReaction({
			type: "fulfill",
			handler: typeof onFulfilled === "function" ? HostMakeJobCallback(onFulfilled) : undefined,
			capability: resultCapability,
		})

		let rejectReaction = new PromiseReaction({
			type: "reject",
			handler: typeof onRejected === "function" ? HostMakeJobCallback(onRejected) : undefined,
			capability: resultCapability,
		})

		promiseData.fulfillReactions.push(fulfillReaction)
		promiseData.rejectReactions.push(rejectReaction)

		// Cancel the reaction directly - no need to notify anyone.
		onCancel(() => {
			promiseData.fulfillReactions.splice(
				promiseData.fulfillReactions.indexOf(fulfillReaction),
				1
			)
			promiseData.rejectReactions.splice(
				promiseData.rejectReactions.indexOf(rejectReaction),
				1
			)
		})
	} else {
		let reason = promiseData.result

		if (promiseData.state === "fulfilled") {
			reaction = new PromiseReaction({
				type: "fulfill",
				handler: typeof onFulfilled === "function"
					? HostMakeJobCallback(onFulfilled)
					: undefined,
				capability: resultCapability,
			})
		} else /* if (promiseData.state === "rejected") */ {
			if (!promiseData.isHandled) HostPromiseRejectionTracker(promise, "handle")
			reaction = new PromiseReaction({
				type: "reject",
				handler: typeof onRejected === "function"
					? HostMakeJobCallback(onRejected)
					: undefined,
				capability: resultCapability,
			})
		}

		let {job, realm} = new PromiseReactionJob(reaction, reason)
		HostEnqueuePromiseJob(job, realm)

		// Cancel the promise reaction job - the host may do anything it wants provided
		// the job is never invoked.
		onCancel(() => {
			HostCancelPromiseJob(job, realm)
		})
	}
}

The former, delay, is obviously userland, but the latter is a rough semi-optimized JS approximation of that spec algorithm. And adding in the onCancel calls in each of those (that function being hypothetical, of course) is concretely what I'm suggesting here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants