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

Use executor return value as oncancel handler in Promise constructor #11

Open
bergus opened this issue Jul 24, 2016 · 12 comments
Open

Use executor return value as oncancel handler in Promise constructor #11

bergus opened this issue Jul 24, 2016 · 12 comments

Comments

@bergus
Copy link
Owner

bergus commented Jul 24, 2016

new Promise(resolve => {
    const timer = setTimeout(resolve, t);
    return reason => clearTimeout(timer);
}, token)

would be an extraordinarily convenient syntax to create cancellable promises.
The returned function would be installed as a cancellation handler on token, and removed from it as soon as resolve or reject are called.
If nothing callable is returned or no token is passed to the constructor it doesn't do anything.

Domenic has brainstormed a Promise.cancellable helper, I have used this pattern before in F-Promise and I have also shown it in the examples. token.subscribeOrCall solves a similar problem.

By only doing anything about the return value when a token is passed, this feature would be fully backwards-compatible.

@ljharb
Copy link

ljharb commented Jul 24, 2016

How would you propose distinguishing that it's a token? instanceof and .constructor are never an option due to cross-realm values (like an iframe).

@bergus
Copy link
Owner Author

bergus commented Jul 24, 2016

@ljharb The general distinction(/casting?) issue is discussed in #3, we can settle on anything there. For the matter of this feature, let's assume that we detect them by checking whether it has a [[Result]] slot (i.e. the internal shape of a CancelToken).
So whatever the detection algorithm decides, the [[CancelToken]] field will be either null or a valid cancellation token, and that's what can be subscribed to or not.

@rtm
Copy link

rtm commented Aug 4, 2016

Funny, I came up with this exact same idea (returning the oncancel handler from the promise constructor body or "executor") when thinking about this myself. Here is a simple userland implementation:

class PromiseCancelledError extends Error {
  constructor(reason) {
    super();
    this.message = reason;
    this.name = "PromiseCancelledError";
  }
}

type Executor<T> = (
  resolve: (value?: T | PromiseLike<T>) => void, 
  reject: (reason?: any) => void
) => Function | void;

function cancellablePromise<T>(executor: Executor<T>, cancelPromise: Promise<any>): Promise<T> {
  return new Promise<T>((resolve, reject) => {
    const onCancel = executor(resolve, reject);

    cancelPromise.then(cancelReason => {
      reject(new PromiseCancelledError(cancelReason));
      if (onCancel) onCancel();
    });

  });
}

I'm by no means any kind of promises guru and I am sure that this approach has lots of holes. The advantage is that is requires no tokens or special cancel calls or extensions to .then or anything else (obviously I'm treating cancellations as rejections).

Simple example:

const cancelPromise = new Promise(resolve => setTimeout(() => resolve("timed out"), 2000));

const promise = makeCancellablePromise(
  resolve => {
    const timer = setTimeout(resolve, 5000);
    return () => clearTimeout(timer);
  },
  cancelPromise
);

Now promise.then(console.log.bind(console), console.log.bind(console)) will report "timed out" after two seconds.

Let me know what I'm missing. I'm not sure, but I think the cancelPromise is somehow token-like in nature, and so the same considerations of who owns it or where it comes from should apply. In any case, this is good enough for an application I'm working on right now where I want to cancel HTTP requests.

I think it would be good enough to put this on the Promise object as Promise.cancellable(...), or perhaps it could be implemented as new CancellablePromise, although I see no way to implement that myself.

I spent some fair amount of time tonight going through your proposal and others and various threads (thanks for the pointers). Frankly, they all confuse me, and I don't think I'm all that easily confused.

@bergus
Copy link
Owner Author

bergus commented Aug 4, 2016

@rtm

I'm by no means any kind of promises guru and I am sure that this approach has lots of holes. The advantage is that is requires no tokens.

Actually, not that much. The idea of simply using promises instead of tokens has been circulated before. It works quite nice! The reasons to use tokens is however that they a) allow synchronous inspection of the cancellation state b) cannot fail c) allow special semantics if you need it.

requires extensions to .then or anything else (obviously I'm treating cancellations as rejections).

Yes, that's where the approach falls a bit short. In general we desire some way to register callbacks (both result and error ones) to not execute their logic when the cancellation is requested. There were multiple approaches presented:

  • .then(fulfill).catch(reject) - promise enters a third downward-propagating state that calls neither, as proposed in Domenic's original draft
  • .then(fulfill).else(reject) - special rejection value that does not call else callbacks (basically a third state in disguise), Domenics current draft
  • .then(x => token.requested ? x : fulfill(x)).catch(e => token.requested ? Promise.reject(e) : reject(e)) - doing it by hand, which I consider quite uncomfortable
  • .then(fulfill, token).catch(reject, token) - what I am proposing here (technically the token in then can be omitted when the promise rejects anyway in case of cancellation)

I spent some fair amount of time tonight going through your proposal and others and various threads (thanks for the pointers). Frankly, they all confuse me, and I don't think I'm all that easily confused.

Can you please let me know where I confused you exactly? I'll try to explain everything and fix the proposal.

@rtm
Copy link

rtm commented Aug 5, 2016

Thank you for your prompt and detailed reply, and sorry to make you rehash stuff that is already out there.

It seems to me that the mismatch is that we may be trying to introduce synchronous paradigms for cancellation into the decidedly asynchronous world of promises. In the link you reference, which purports to justify why synchronous promise state examination is necessary--something we'd tell any beginner is a no-no--I don't understand why the problem could not be solved with the addition of one additional await on the second line:

async function f(cancel) {
  let canceled = false;
  await cancel.then(_=> canceled = true);
  await cheapOperation(cancel);
  if (canceled) throw new CanceledOperationError();
  await expensiveOperation(cancel);
}

Perhaps we need to examine more closely the trade-off between implementation cost in the broad sense--by which I mean the introduction of new machinery, object types, and signatures, both in terms of the coding-level implementation cost and the cost of people learning the new elements--and functionality including the handling of edge cases, if I may call them that. From a political perspective, I think proposals involving such new machinery are likely to be the subject of endless bikeshedding and people talking past each other, and suffer the fate of many other ES proposals which is to fade into oblivion.

Yes, that's where the approach falls a bit short. In general we desire some way to register callbacks (both result and error ones) to not execute their logic when the cancellation is requested.

I think I understand this, but if the rejection caused by a cancellation has a reason which is an identifiable type, it can be handled by checking the type:

.then(fulfill).catch(e => {
  if (e instance of PromiseCancelledError) {
    console.log("Hey, promise was cancelled due to", e.message, "but that's OK");
  } else {
    console.log("THE SKY IS FALLING", e);
    rethrow or something;
  }
})

Which although a bit clumsy seems preferable to new types, new states, and/or new methods on Promise.prototype.

I think there is some generic lesson to be learned here from observables. Observables do not involve any separate machinery for cancelling or terminating or throwing. Other observables themselves are used for such things. So one observable can be combined with another one in such a way that something emitting on the first shuts down the second, etc.

@bergus
Copy link
Owner Author

bergus commented Aug 5, 2016

@rtm

we may be trying to introduce synchronous paradigms for cancellation into the decidedly asynchronous world of promises. The link you reference purports to justify why synchronous promise state examination is necessary--something we'd tell any beginner is a no-no.

The reason we tell beginners to avoid inspection is because they tend to use it where it is unnecessary and would only complicate things.
I do believe that cancellation is something synchronous. Once you call cancel(), no further work should be started in any case, from right now on. We don't even want to wait for a microtask go-around, making cancellation immediate does simplify its semantics.
This does not mean that subscribers to the cancellation event should be called synchronously, but that testing whether cancellation was requested should possible synchronously. It also is comfortable, otherwise you would be going to manually use a canceled flag such as the one in the snippet above.

I don't understand why the problem could not be solved with the addition of one additional await on the second line await cancel

That cannot work. It would block the code until the cancellation is requested, and not execute the actual operation at all.
Cancellation is a race - does the cancellation complete first or is it cancelled before the result is ready?
They need to be both waited for independently - concurrently - and there does not exist an equivalent synchronous syntax structure for this. Some drafts of how it could look with async/await can be found here.

if the rejection caused by a cancellation has a reason which is an identifiable type, it can be handled by checking the type. Which although a bit clumsy seems preferable to new types, new states, and/or new methods on Promise.prototype.

Uh, an identifyable type does need a new type. Especially if there is supposed to be a try-syntax for it.
And yes, it is horribly clumsy. Often you want to catch errors and not cancellations (if those are "catchable" or downwards-propagating at all), and this introduces a lot of boilerplate. Which is why Domenic's proposal introduces the new else method as sugar for this type check branching.

And also I believe that cancellation as a rejection type does not solve the chaining problem at all. It works well if you are the subscriber of a cancellable promise, who just wants to distinguish the outcome.
It does not work for you when you are the worker. If we consider a standard promise chain,

A().then(B).then(C).then(showResult).catch(showError)

then the behaviour we want is that

  • when we cancel while A is running, we don't want B, C and showResult to run
  • when we cancel while B is running, we don't want C and showResult to run
  • when we cancel while C is running, we don't want showResult to run
  • when we cancel after A/B/C were run, we want showResult to have run an cancellation not to have an effect any more.

Also we typically want the showError or a cancellation handler to run without waiting for the current operation to finish. So if we use a cancel promise that rejects when we request cancellation, the code actually looks like this:

Promise.race([cancel,
              Promise.race([cancel,
                            Promise.race([cancel,
                                          A()
                            ]).then(B)
              ]).then(C)
]).then(showResult).catch(showError)

This is where Domenic's proposal falls short, and my approach has its foundations:

A().then(B, cancel).then(C, cancel).then(showResult, cancel).catch(showError)

@rtm
Copy link

rtm commented Aug 6, 2016

Uh, an identifiable type does need a new type. Especially if there is supposed to be a try-syntax for it. And yes, it is horribly clumsy. Often you want to catch errors and not cancellations (if those are "catchable" or downwards-propagating at all), and this introduces a lot of boilerplate.

I meant a new first-class type, not one new error type. As for boilerplate, by definition it can always be abstracted away. Just as an example, one can imagine a catch handler factory perhaps called catchCancelled, either built-in or more likely written by users, which would be used as

cancellablePromise.catch(catchCancelled(...)).then(...)

which would be implemented as

function catchCancelled(handler) {
  return function(reason) {
    if (reason instanceof CancelledPromiseError) return handler(reason);
    else throw reason;
  };
}

That seems acceptably streamlined and less intrusive than new methods on the Promise prototype and/or changes to the then or catch signature.

So let's move on to the interesting stuff which is cascading of cancellations down the chain.

A().then(B).then(C).then(showResult).catch(showError)

We assume that A, B and C are something like

function A() { return makeCancellablePromise(executorA, cancelPromiseA); }
function B() { return makeCancellablePromise(executorB, cancelPromiseB); }
function C() { return makeCancellablePromise(executorC, cancelPromiseC); }

If cancelPromiseA fires and A is cancelled, or cancelPromiseB fires and B is cancelled, or cancelPromiseC fires and C is cancelled, then nothing happens except the showError, as desired. In this example, there are separate cancellation promises for A, B, and C. But there's no reason it could not be the same one:

function A() { return makeCancellablePromise(executorA, cancelPromise); }
function B() { return makeCancellablePromise(executorB, cancelPromise); }
function C() { return makeCancellablePromise(executorC, cancelPromise); }

The issue here is of course is that A, B, and C must be "prepared in advance" as cancellable promises with an explicit pre-specified associated cancelling promise, as opposed to being indicated as being cancellable at "run-time" by adding the cancel token as a second/third argument to then in the chain. But it seems possible to me that this could even be preferable; what if B wants to refuse to ever be cancelled, even if it is called in a then handler with a cancel token (or did your proposal address this scenario?).

In any case, in the realm of syntactic sugar, one could image machinery to ease this requirement in the form of utilities (again, either built-in or written by users), such as

function makeMultipleCancellablePromises(executors, cancelPromise) {
  return executors.map(executor => makeCancellablePromise(executor, cancelPromise));
}

the result of which could be run by

chainPromises(makeMultipleCancellablePromises([executorA, executorB, executorC], cancelPromise))
  .then(showResult)
  .catch(catchCancellable(() => console.log("something was cancelled!"))
  .catch(showError);

Well, I'm sort of running out of gas here, and none of this addresses the need to cancel a promise RIGHT NOW if that is considered non-negotiable. Still, I continue to believe that this sort of approach could be the right trade-off between complexity and functionality, and between baked-in features and features left up to users to wrap.

@bergus
Copy link
Owner Author

bergus commented Aug 6, 2016

The issue here is of course is that A, B, and C must be "prepared in advance" as cancellable promises with an explicit pre-specified associated cancelling promise

Indeed. If they are cancellable functions, the chain would look like

A(token).then(x=>B(x, token)).then(y=>C(y, token)).

But what I am mostly concerned about functions that are not cancellable, or not asynchronous at all. What if I did request(…, token).then(JSON.parse)? Or something very common like synchronous logic parts in a handler: .then(x => { console.log(x); return B(x, token) })?
I want to ensure that not even this synchronous logic is run when the cancellation is requested - and that's what passing the token to then enables.

It seems possible to me that this could even be preferable; what if B wants to refuse to ever be cancelled, even if it is called in a then handler with a cancel token (or did your proposal address this scenario?).

If B does not want to be cancelled (or it simply is not possible), it would not take a token. In my proposal, if you do ….then(B, token) it only means that B is not called in the first place when cancellation had been requested, but it is not affected at all once it is running.
The only thing that token influences when B is already running is that its results might be ignored, since it basically equivalent to Promise.race([….then(x => token.requested && B(x), token.promise]).

one could image machinery to ease this requirement in the form of utilities (again, either built-in or written by users)

Exactly that is what my proposal is, a utility for cancellation (tokens) built into then :-)
We need this to be in the (or at least, a) standard, as otherwise implementations would diverge into incompatibilities.

@ljharb
Copy link

ljharb commented Aug 6, 2016

Also, note that instanceof doesn't work across realms (iframes, web workers, node's vm module) so any mechanism of brand-checking that relies on instanceof or .constructor === isn't suitable.

@rtm
Copy link

rtm commented Aug 7, 2016

Here is something: https://github.com/rtm/cancelable-promises

@rtm
Copy link

rtm commented Aug 17, 2016

In case you're interested, I've completely re-engineered my approach, which now treats canceled promises as being in a permanently pending state:

https://github.com/rtm/cancelable-promise

@bergus
Copy link
Owner Author

bergus commented Aug 17, 2016

Interesting, but I've already made a case for rejection.

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

No branches or pull requests

3 participants