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

Draft C #7

Open
ForbesLindesay opened this issue Feb 2, 2013 · 13 comments
Open

Draft C #7

ForbesLindesay opened this issue Feb 2, 2013 · 13 comments

Comments

@ForbesLindesay
Copy link
Member

Based on discussions in #6

Terminology

In addition to the terminology from Promises/A+ we use the following:

  1. CancellationError is an error used to reject cancelled promises.
  2. onCancelled is a function associated with a promise (in an implementation specific manner) that is used to support cancellation propagation as described.

Requirements

Promises can be created in two ways:

Directly created promise

These are promises that are created directly using the API for creating a new promise exposed by the library.

Cancellable promises have an additional onCancelled function provided by the creator of the promise. This can be a no-op, but should typically be used to cancel the underlying operation (e.g. an ajax request).

Directly created promises have a cancel method.

When cancel is called on a pending promise it:

  1. calls onCancelled with no arguments
  2. if onCancelled throws an error it rejects the promise with that error.
  3. If onCancelled does not throw an error it rejects the promise with a CancellationError

Promise created by then

When then is called, it creates a new promise. Call this the output promise. Call the promise that then was called on the source promise. Call any promise returned by a callback or errorback a child promise:

var output = source.then(function (res) { return childPromise; },
                         function (err) { return childPromise; });

If source is a cancellable promise then output must be a cancellable promise. The cancel methd on output must have the same behaviour as for a 'Directly created promise'. The onCancelled method associated with the output promise must have the following behaviour:

  1. Call cancel on the source promise.
  2. Call cancel on any child promises.

The CancellationError

When a promise is directly cancelled it is rejected with a CancellationError error. A CancellationError must obey the following criteria:

  1. It must be an instance of Error (cancellationError instanceof Error === true).
  2. It must have a name property with value "cancel".

Recommended Extensions

The following two extensions may be provided, and if provided should behave as follows:

Uncancellable

unacnellable should return a new promise which will be fulfilled or rejected in the same way as the current promise but does not have a cancel method or has a no-op in place of cancel.

Fork

fork should return a new cancellable promise (output) which will be fulfilled or rejected in the same way as the current promise (source) but where it won't cancel the source if the output promise is cancelled.

Sample Implementation

An example implementation may make the behavior described above easier to follow.

The following sample extends Promise to produce CancellablePromise and uses ES6 syntax:

class CancellablePromise extends Promise {
  constructor(fn, onCancelled) {
    super(fn);
    this._onCancelled = onCancelled;
  }

  cancel() {
    if (this.isPending() && typeof this._onCancelled === "function") {
      try {
        this._onCancelled();
      } catch (e) {
        this._reject(e);
      }
    }

    this._reject(new Cancellation()); // double-rejections are no-ops
  }

  then(cb, eb, ...args) {
    var source = this;
    var children = [];
    function wrap(fn) {
      return function (...args) {
        var child = fn(...args);
        if (isPromise(child) && typeof child.cancel === 'function') {
          children.push(child);
        }
        return child;
      }
    }
    return new CancellablePromise(
      resolve => resolve(super(wrap(cb), wrap(eb), ...args)),
      () => {
        for (var child of children) child.cancel();
        source.cancel();
      }
    );
  }

  //optional extension
  uncancellable() {
    return Promise.prototype.then.call(this);
  }

  //optional extension
  fork() {
    return new CancellablePromise(resolve => resolve(this), noop);
  }
}
@novemberborn
Copy link

I don't understand the difference between uncancellable and fork. In both cases you get a promise whom's cancellation does not affect the source promise, and can therefore safely be handed to out to untrusted consumers.

@ForbesLindesay
Copy link
Member Author

Fork returns a promise which can be cancelled:

var cancellable = getCancellablePromise();
var derivative = cancellable.fork();
derivative.cancel();
derivative
  .then(null, function (err) {
    Assert.equal(err.name, 'cancel');
    console.log('yay, we cancelled it');
  });
cancellable
  .then(function (err) {
    console.log('yay, we didn't propagate the cancel');
  });

Uncancellable returns a promise that can't be cancelled at all

var cancellable = getCancellablePromise();
var derivative = cancellable.uncancellable();
derivative.cancel();
// => throws error allong the lines of "Object 'derivative' has no method 'cancel'"

@novemberborn
Copy link

Uncancellable returns a promise that can't be cancelled at all

I assume that behavior propagates through further calls to then?

var cancellable = getCancellablePromise();
var derivative = cancellable.uncancellable();
var deriv2 = derivative.then(function(x){ return x; });
deriv2.cancel();

Depending on the implementation that may be hard to enforce. So it's good you're leaving it as an extension. I don't really see the need for it though, what do you care if the derived promise you're handing out gets cancelled?


The cancel methd on output must have the same behaviour as for a 'Directly created promise'. The onCancelled method associated with the output promise must have the following behaviour:

Call cancel on the source promise.
Call cancel on any child promises.

This definition is problematic. It doesn't actually define expected behavior:

var output = source.then(onFulfilled, onRejected);

output will be pending if a) source is pending, or b) child is pending. Note that there can be only one child. These scenarios should be listed more explicitly, with prose, rather than pseudo-code. Why even call source.cancel if output is pending due to child?

Furthermore, propagating cancellation is implementation specific, so even saying "Call cancel on the source promise" seems wrong.


WIP implementation at https://github.com/novemberborn/legendary/tree/cancellation-c with some examples at https://gist.github.com/novemberborn/5282453.

@ForbesLindesay
Copy link
Member Author

You care because you may want to hand one promise to multiple consumers. You could do this by creating lots of separate forks and handing one to each.

I accept the criticism that it's not the easiest thing to interpret I have zero experience writing specs.

@novemberborn
Copy link

Having the cancellation behavior encapsulated in a subclass is interesting, though it also means you have to take more care in composing promise chains. E.g.:

server.on("request", function(req, res){
  var p = app(req);
  req.on("close", p.cancel);
  p.then(function(response){
    res.writeHead(response.status, response.headers);
    res.end(response.body.join(""));
  });
});

If you can't rely on app() returning a CancellablePromise you have to wrap it yourself:

server.on("request", function(req, res){
  var p = new CancellablePromise(function(resolve){
    resolve(app(req));
  });
  req.on("close", p.cancel);
  p.then(function(response){
    res.writeHead(response.status, response.headers);
    res.end(response.body.join(""));
  });
});

And actually this goes for any promise composition, e.g.:

var c = a().then(function(){
  return b();
});

If a() happens to return an uncancellable promise, calling c.cancel() won't stop b() from being called, or cancel its execution.

I suppose this is a trade-off between cancel propagation having unintended side-effects and being super explicit about how to construct a cancellable promise chain? I also suspect libraries that implement CancellablePromise should use it for composition helpers such as all, such that users can explicitly opt out of it by calling uncancellable().

@novemberborn
Copy link

onCancelled is a function associated with a promise (in an implementation specific manner) that is used to support cancellation propagation as described

Why not standardize on this? It'd make switching libraries easier. One suggestion may be to return the onCancelled from the resolver?

My latest implementation attempt is at https://github.com/novemberborn/legendary/compare/cancellation-c-attempt-2, it should support CancellablePromise, CancellationError, uncancellable and fork.

@ForbesLindesay
Copy link
Member Author

Yes, you have to be careful because not all promises will support cancellation. This goes for any promise extension though, and is not something special about cancellation. I should add that the cancellation as a separate class is just a tool to explain how it might be implemented. My own "minimal" promise implementation will still include cancellation in core once cancellation is fully specified.

With regards to helpers such as all. They should propagate cancellation wherever possible.

The reason for not specifying how onCancelled is attached is two fold:

  1. It depends on the resolvers spec, which is a fair way off being finished at the moment.
  2. It's essentially an internal method (once set), so it could be private in some libraries, but others may choose not to make it private for performance reasons.

In short, I think it should be specified, but not until some time in the distant future.

I look forward to looking at the implementation.

@novemberborn
Copy link

I should add that the cancellation as a separate class is just a tool to explain how it might be implemented. My own "minimal" promise implementation will still include cancellation in core once cancellation is fully specified.

Yea, fair enough. I do think I have to go the subclass route if I want uncancellable to work in Legendary, but maybe legendary.Promise should be the cancellable one, with another UncancellablePromise.

@ForbesLindesay
Copy link
Member Author

Personally I was thinking of just creating a new promise (call it p) and then doing p.cancel = null to make it "uncancellable", but that won't be in core.

@novemberborn
Copy link

Took me a while to get everything in working order, but check out https://github.com/novemberborn/legendary/compare/cancellation-c. It's got some tests too!

One tricky edge-case I found is synchronously cancelling a promise before the callbacks have executed, e.g.:

var derived = promise.then(doSomething);
derived.cancel();

IMO doSomething() should still be executed but its result discarded. It'd be too surprising if sometimes your callbacks don't run.

@ForbesLindesay
Copy link
Member Author

Nope, the cancellation propagates to promise and doSomething is never called, simple.

@novemberborn
Copy link

Sorry, I forgot to add that this is the case where promise is already fulfilled and thus cannot be cancelled, and the code is aware of it having been fulfilled without being able to observe cancellation as easily.

@arokor
Copy link

arokor commented Apr 10, 2014

For directly created promises, let's assume that the cancellation of a promise requires some async work. Wouldn't it make sense then to let onCancelled optionally return a promise.

var base = Promise(function(resolve, reject){...}, function(){
  return new Promise(...);  // resolved when cancellation is finished
});

So the addition to the 3 rules for when calling cancel on a pending promise would then be something like:
4. If onCancelled returns a promise the base promise is rejected with CancellationError when the returned promise resolves. If the returned promise is rejected with error err the base promise gets rejected with err.

mtdowling added a commit to guzzle/promises that referenced this issue Mar 8, 2015
- This commit adds constants for each promise state.
- Removing the "cancelled" state in favor of rejecting promises with a
  CancellationException. Trying to follow
  promises-aplus/cancellation-spec#7
- Throwing exceptions when you try to fulfill/reject FulfilledPromise and
  RejectedPromise.
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

3 participants