-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
This seems to conflate the two distinct uses for "cancellation" - "registering disinterest", ie, an An individual Promise is supposed to confer no privileges except reacting to the final fulfillment - as such, a |
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. |
The only thing a method on a Promise should do imo is "unthen" - which should not notify the creator of the Promise, for example. |
@ljharb for what it's worth that issue is pretty simple and can be addressed:
If these async functions returned something that is not a Promise but is either a subclass (worse) or a Specifically in the context of this proposal:
With a 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, |
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). |
While I agree with:
I disagree that implies they shouldn't be cancelable, in fact given However I don't think someone who has reference to a Promise should be able to cancel it. I think 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 For example through the new metaproperty like 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(); |
If you already hold the 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 I've done an awful lot of cancellation (via 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 |
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 |
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. |
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 |
@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. |
@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.
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 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 Several places in JavaScript (namely generators) already have third state. To sum up my opinion:
|
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. 😢 )
Thank you @benjamingr for coming up with this example:
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 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".
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 ...
Most of my proposal was supposed to focus on preventing to manually passing down that token to all sub-operations. @getify notes:
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 With this in mind, looking at the alternative syntax proposed: async function dobuleTimeout() {
await delay(1000);
await delay(1000);
} here async function dobuleTimeout() {
const p = delay(1000)
await p
await delay(1000)
} ... and that would mean that every call inside of 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 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 async@token function foo() {
token.register(...)
}
async function bar () {
// Doesn't use the token, token is not created.
} Having it be a separate new Promise((resolve, reject, token) => {}) this could avoid the aforementioned debugging hell by moving the magic from a function call to the
Sorry for the confusion, in my initial proposal I was trying to suggest that the inappropriately named function cancel(token) {
bindTokens(token, this.token)
return this
} where 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 namingI chose Also I went with a placeholder
Honestly, I hadn't thought of this. Thank you for pointing it out! To make sure that the creator stays in control, 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
I didn't understand the purpose of If I understand you right then
My naming might have caused this confusion: I am sorry 😓 . The third state should imo. be a separate discussion. |
Yes, it would simply pass it down the stack, everything on the call stack gets the cancelToken.
The idea of this is to only propagate cancellation, as such to avoid people abusing it as a general purpose context function b() {
console.log(function.cancelToken.foo); // undefined
}
function a() {
const { cancelToken } = CancelToken.create();
cancelToken.foo = 'foobar';
b.start(cancelToken);
} |
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. |
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:
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:
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 I would not pass the token to |
Just throwing this out there, but it'd be cool if we could pass a token as the third argument to It'd be cool if thennables that support this third parameter could then pass it on to any calls to So when you call an async function, you can pass the token to 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 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 |
@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 |
@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. |
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, |
@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 |
@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
And BTW, using cancellation would make useless things like const p1 = a.then(foo)
const p2 = a.then(foo)
a.unthen(foo) // ??? |
Maybe a poll? Also, it might be worth doing a couple things special in that poll:
|
It doesn't mutate it in a way that's observable. You can't tell, in your example, whether |
@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 |
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. |
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. |
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 :-/ |
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
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, |
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:This could be built in with
Promise.race
where the promises after the first promise get automatically cancelled.The text was updated successfully, but these errors were encountered: