A simpler idea?

[domenic]

Inspired by #1 (comment), what about something as simple as this? Assume underscored properties are "actually private," i.e. wouldn't be specced and are only here to illustrate. Using ES6 for brevity.

class CancellablePromise extends Promise {
  constructor(resolver, onCancelled) {
    this._onCancelled = onCancelled;
    this._isResolved = false;
    super((resolve, reject) => {
      this._reject = reject;
      const wrappedResolve = value => { resolve(value); this._isResolved = true; };
      const wrappedReject = reason => { reject(reason); this._isResolved = true; };
      resolver(wrappedResolve, wrappedReject);
    });
  }

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

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

  then(...args) {
    // When you chain off a cancellable promise, you get another cancellable promise.
    // And cancellations propagate to progenitor promises, i.e. when calling
    // `promise.then(...).cancel()`, this ends up calling `promise.cancel()` too (see examples).
    return new this.constructor(
      (resolve, reject) => super.then(resolve, reject),
      () => this.cancel()
    );
  }

  // Allow removing cancellation privileges, for handing out to untrusted consumers. 
  uncancellable() {
    return super.then();
  }
}

// A cancellable XHR.
function get(url) {
  const xhr = new XMLHttpRequest();
  return new CancellablePromise(
    (resolve, reject) => {
      xhr.open("GET", url, true);
      xhr.onload = () => resolve(request.responseText);
      xhr.onerror = () => reject(new Error("Can't XHR"));
      xhr.send();
    },
    () => xhr.abort()
  );
}

// A cancellable JSON-parsing XHR, using cancellation propagation.
function getJSON(url) {
  return get(url).then(JSON.parse);
}

getJSON("http://example.com/data.json").cancel();
// => should work, calling the original promise's `cancel`, and thus
// the `onCancelled` passed in to the constructor, and thus `xhr.abort`.

get("http://example.com/data.json").uncancellable().cancel();
// => throws, no `cancel` method

[ForbesLindesay]

That's nice, I like the idea of just explicitly saying .uncancellable. 2 cases that need proper treatment come to mind:

Case 1:

var prom = get('https://foo.com/1')
  .then((res) => get('http://foo.com/' + res));
...
prom.cancel();

Is there any circumstance in which the second promise gets properly cancelled?

Case 2:

I think uncancellable probably isn't quite the right behaviour, I think ideally it would be:

  uncancellable() {
    return new CancellablePromise(
      resolve => resolve(this),
      () => {}
    );
  }

That is to say we should still return a promise that can be rejected with an OperationCancelled exception by calling cancel. I then think we'd need a new name for uncancellable since it wouldn't be an apt description of the behavior.

[juandopazo]

It looks good so far. I'm not thrilled about calling super in then that way because it's creating yet another intermediate promise. There could be a better way. But besides that it looks like how I'd implement it.

[skaegi]

Your "then" implicitly supports parent cancellation instead of just cancelling the work you added by calling "then".
If I'm understanding your use of super and resolve correctly I think you just want:

then(...args) {
  return new CancellablePromise(
    resolve => resolve(super(...args))
  );
}

[domenic]

@skaegi Implicitly supporting parent cancellation is the entire goal: otherwise getJSON(url).cancel() does not cancel the underlying XHR.

[domenic]

@ForbesLindesay

Case 1

That's an interesting one we should probably make possible and think more about. Good catch.

Case 2

The point of uncancellable was to revoke the ability of promise consumers to interfere with promise state, so I don't think we want to vend CancellablePromises from it.

[skaegi]

If that's what you want then write:

function getJSON(url) {
  var xhr = get(url);
  var result = xhr.then(JSON.parse);
  result.cancel = function() {
    xhr.cancel();
    result.cancel();
  }
}

Alternately....
If we want to say that parent cancellation is the default how can I just cancel the operation I added via then

[skaegi]

sigh

function getJSON(url) {
   var xhr = get(url);
   var result = xhr.then(JSON.parse);
   var resultCancel = result.cancel.bind(result);
   result.cancel = function() {
     xhr.cancel();
     resultCancel();
   }
 }

[ForbesLindesay]

@domenic I see your point re case 2

@skaegi you've just demonstrated the problem. If it's anywhere near that difficult to propagate cancellation people won't bother to do so unless they personally need cancellation in their app, which would mean cancellation was never truely available to consumers of third party libraries. It needs to propagate by default, but propagation needs to be trivial to stop.

[skaegi]

Here's a simpler and probably better variation that doesn't override cancel

function getJSON(url) {
  var xhr = get(url);
  return xhr.then(JSON.parse, function(error) {
    if (error && error.name='Cancel') {
      xhr.cancel();
    }
    throw error;
  });
}

@ForbesLindesay Our app is very much in the space that needs cancellation and is something we both use and test every day. That absolutely means that we're willing to write code like above when we're writing a "filter" however far and away the more typical case is where we've add a new operation to the chain. Whatever the propagation behavior I want to make sure it's easy to cancel just that operation without impacting the parent and the rest of it's children.

Our Deferred implementation used parent propagation for the last six months and we ultimately removed it and are much happier for it. Now when an operation is not cancelling correctly we can actually track it down instead of trying to understand why seemingly unrelated sibling calls are being rejected. With that said I'm not militantly opposed to the idea and will certainly follow whatever it is we standardize here. It would however really help if I could see a code snippet that showed how to localize the cancel propagation to just the operation then-ed to the chain.

[domenic]

@skaegi

It would however really help if I could see a code snippet that showed how to localize the cancel propagation to just the operation then-ed to the chain.

So your desired semantics are something like this?

get("http://example.com").then(function () {
  return get("http://example.org");
}).cancel(); // should only cancel the get to example.org, not example.com

?

[skaegi]

Yes that is correct.

Let me try to cover the cases as I see them...

1. For the case where the parent is pending a cancel call on a promise created by a then would call its onRejected handler with a Cancel Error. This then promise should also be removed as a listener to the parent to prevent an additional callback when the parent completes.

For your example above where the onRejected handler is undefined, the reject would propagate normally and the call to get("http://example.org") would never happen at all.

2. For the case where the parent has completed but the onFulfilled/onRejected handler for the then promise has returned a promise, the then promise assumes this state (e.g. as per the promises spec). In that case a cancel call will occur on this newly assumed promise.

For your example above this is the case where the onFulfilled handler returns the promise from get("http://example.org") and it is still pending. A cancel call in this case results in a Cancel Error getting propagated and depending on the implementation the XHR might choose to abort.

3. For the case where the promise created by a then is already completed the cancel call is a noop.

[ForbesLindesay]

If we replace then witht eh following, we track all the promises that appear inside of the then call, so that they are also cancelled when .cancel is called.

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

The behavior is then:

get("http://example.com")
.then(function () {
  return get("http://example.org");
})
.cancel(); // should ancel the get to example.com or example.org
           // (depending on which request is currently in progress)

To support @skaegi's request:

Add a method fork:

fork() {
  return new CancellablePromise(resolve => resolve(this), noop);
}

Which can be used:

get("http://example.com")
.fork()
.then(function () {
  return get("http://example.org");
})
.cancel(); // should only cancel the get to example.org, not example.com

[ForbesLindesay]

And equally:

get("http://example.com")
.then(function () {
  return get("http://example.org");
})
.fork()
.cancel(); // shouldn't cancel any requests, but still results in a rejected promise with the
           // cancellation exception

and

get("http://example.com")
.then(function () {
  return get("http://example.org");
})
.uncancellable()
.cancel(); // Truly doesn't cancel anything, does `cancel` even exist here?

[ForbesLindesay]

To make explanations clearer, I've separated out the promises so each one that gets created has its own name.

var A = get("http://example.com");
var B;
var C = A.then(function () {
  B = get("http://example.org");
  return B;
});

//some time later...
C.cancel();

Case 2

This seems fine:

1. A is resolved (successfully)
2. C assumes the state of B
3. C is cancelled, which cancels B because C has assumed B's state.

Case 3

This also seems fine:

1. A is resolved
2. C assumes the state of B
3. B is resolved
4. C is resolved because of 2 & 3
5. C's cancel method is called (which is a noop because C is already resolved).

Case 1

All promises are pending. C has not assumed any state.

Without propagation:

1. C is cancelled
2. Cancellation results in C being rejected
3. A resolves (successfully)
4. A calls it's then handlers
5. B is created and ultimately resolves
6. B's resolution is ignored.

With propagation:

1. C is cancelled
2. A is cancelled by propagation from C
3. B is never created
4. A and C are both rejected with a cancellation exception.

Your proposed flow

1. C is cancelled
2. Cancellation results in C being rejected
3. C magically inserts a new promise in between A and B and never resolves that promise
4. A is resolved
5. B is never created

alternatively

1. C is cancelled
2. Cancellation results in C being rejected
3. C magically replaces the callback and errback functions in the correct one of A's registered handlers with a no-op
4. A is resolved
5. B is never created

[ForbesLindesay]

With the use of the fork method we get the behavior @skaegi desires (but with an explicitly created promise to not resolve, rather than having to magic one out of thin air):

var A = get("http://example.com");
var B = A.fork();
var C;
var D = B.then(function () {
  C = get("http://example.org");
  return C;
});

//some time later...
D.cancel();

case 1

1. D is cancelled
2. B is cancelled by propagation from D
3. C is never created
4. B, C and D are all rejected with a cancellation exception.
5. A is resolved

case 2

This seems fine:

1. A is resolved (successfully)
2. B is resolved (successfully) with the state of A
3. D assumes the state of C
4. D is cancelled, which cancels C because D has assumed C's state
5. D and C are rejected with the cancellation exception

case 3

1. A is resolved (successfully)
2. B is resolved (successfully)
3. D assumes the state of C
4. C is resolved
5. D is resolved because of 3 & 4
6. D's cancel method is called (which is a noop because D is already resolved).

[skaegi]

Thanks @ForbesLindesay that all makes sense to me and covers the cases I care about. I personally think both uncancellable and fork are not strictly necessary but apart from that I think we're ready for a Draft C.

[ForbesLindesay]

Excellent, I think we're definitely moving towards agreement here.

The use cases that may necessitate both fork and uncancellable:

function asyncMethodA() {
  return somethingIDontWantCancelled()
    .fork()
    .then(somethingThatCanBeCancelledSafely);
}
var A = asyncMethodA();

// Some time later

A.cancel();//should only cancel second part of A

function asyncMethodB() {
  return somethingThatINeedToShare()
    .uncancellable();
}

var B = asyncMethodB();

//some time later

thirdPartyCodeIShouldBeSuspiciousOf(B);

//some time later
B.then(function (res) {
  //do something important
  //better hope third party code
  //didn't cancel B
});

It would be good to flesh out what we think would be best for these two cases before the next strawman spec.

P.S. I really like @domenic's idea of doing the strawman as just some source code with the appropriate behavior, it's a lot easier to see what's going on sometimes.

[skaegi]

Would inlining work? It would require directly creating new promises but I was hoping to minimize API on promise

function asyncMethodA() {
  // fork
  return new CancellablePromise(resolve => resolve(somethingIDontWantCancelled()), noop)
    .then(somethingThatCanBeCancelledSafely);
}

function asyncMethodB() {
  // uncancellable
  return Promise(resolve => resolve(somethingThatINeedToShare()));
}

[skaegi]

Here's a first crack at a draft C
https://gist.github.com/4694432

[ForbesLindesay]

That has nothing in it about propagation

[skaegi]

re: That has nothing in it about propagation
@ForbesLindesay Indeed -- I wanted to think a bit more about the child propagation I see in your then implementation above but then ran out of time. I'd definitely missed that on first read and had thought there was just regular then promise assumption and did not realize the promise would be wrapped to retain a reference to the "source".

Is the intent there to have a cancel call propagate to the children if the source has already been fulfilled?

[ForbesLindesay]

Yes, if source is unfulfilled, propagate to source if source is fulfilled, propagate to each child

[skaegi]

If source is fulfilled and then later cancelled I think the resulting state is a bit odd.

What happens when a new child is thened to source?
I believe we would first fulfill it and then if that call returns a promise cancel it. The problem there is that it implies that we have introduced this new "cancelled" side state (e.g. a non-pending promise can be cancelled or not cancelled in addition to fulfilled or rejected.) Alternately, we could eliminate this state by saying that the cancel call propagates to just the set of children at the time of the call but that seems inconsistent.

I buy that we might want this aggressive cancel behavior in some cases and you've shown how one might implement it however this feels to me like one of a range of implementation choices for a cancellable promise.

This might not be popular however I feel the best we can do is say that cancel attempts to cancel a pending promise, suggest propagation might occur, but then leave it up to the implementation to figure out what that means instead of trying to specify it. Even without being prescriptive on propagation there is still value in the cancel semantics that have been worked out here and we should try to spec it and then stop and see where we are.

[domenic]

I'm pretty sure the idea was that cancellation is just a type of rejection, and that if you're already fulfilled or rejected, cancellation should have absolutely zero impact.

[domenic]

Ah, I see, the original post doesn't reflect that---this._onCancelled still gets called. I'll edit that.

[ForbesLindesay]

@skaegi I think you're missing something here and might want to re-read it a bit more carefully.

var source = new Promise();
var outputA = source.then(function () { return childA; });
var outputB = source.then(function () { return childB; });

If source is pending and we cancel source, it calls onCancelled for source and then rejects source with a cancellationError. Nothing else happens other than normal rejection propagation.

If source is fulfilled and then cancel is called on source it is a no-op. Literally nothing should happen.

If source is pending and one of outputA or outputB is cancelled, then source is cancelled via propagation. source is then rejected with a CancellationException which causes normal rejection propagation and rejects outputA and outputB.

If source is fulfilled but childA and childB are pending and outputA is cancelled, outputA propagates that cancellation to childA and childA becomes rejected, which leads to outputA being rejected. outputB is unaffected and can be resolved once childB (also unaffected) gets resolved.

[skaegi]

Hmm... that sounds exactly right to me (nearly Cancellation axioms) so I think I must be getting confused by the sample code and its use of children/parents in the then implementation.

Below is the (translated) code I have in my implementation. Does this do the same thing or is there a case where there are multiple children in that children/parents array?

then(cb, eb, ...args) {
  var _cancel = this.cancel.bind(this);
  function wrap(fn) {
    return function (...args) {
      var res = fn(...args);
      if (isPromise(res)) {
        _cancel = (res.cancel) ? res.cancel.bind(res) : function(){};
      }
      return res;
    }
  }
  return new CancellablePromise(
    resolve => resolve(super(wrap(cb), wrap(eb), ...args)),
    () => {
      _cancel();
    }
  );
}

Regardless, apologies as I think there is no problem here and we're in agreement on a reasonable default propagation mechanism.

---

The other issue I wanted to cover was onCancel. In the current Draft C if onCancel throws we reject with that exception instead of the CancellationError.

Is the intent there to give the onCancel call a measure of control over how the promise is rejected or is it to help with error handling that occurred in 'onCancel'. If it's the former I think we need a use case to show why this is helpful because being able to consistently distinguish a CancellationError (and potentially propagate and handle it further) is really useful. If it's the latter then I think this should instead be handled in an implementation specific manner as again giving code further down the chain a chance to handle Cancellation is useful.

[ForbesLindesay]

Looks equivalent to me. I think you're correct that children is only ever a single element array.

[skaegi]

For specification purposes what if we didn't have onCancel at all and stipulated that cancel always throw a CancellationError. One could then write the minimal implementation as follows...

class CancellablePromise extends Promise {
  constructor(fn) {
    super(fn);
  }

  cancel() {
    // Ideally... if (pending) return
    var cancellationError = new Error();
    var cancellationError.name = 'Cancel';
    this._reject(cancellationError);
  }

  then(cb, eb, ...args) {
    var _cancel = this.cancel.bind(this);
    function wrap(fn) {
      return function (...args) {
        var res = fn(...args);
        if (isPromise(res)) {
          _cancel = (res.cancel) ? res.cancel.bind(res) : function(){};
        }
        return res;
      }
    }
    var promise = new CancellablePromise(
       resolve => resolve(super(wrap(cb), wrap(eb), ...args)
    );
    promise.cancel = function() {
      _cancel();
    );
    return promise;
  }
}

A promise implementation "could" of course add an 'onCancel' to their constructor however this would be a convenience and just sugar for:

cancellablePromise.then(null,  function (error) {
  if (error.name === 'Cancel') {
    onCancel();
  }
})

[domenic]

The point of onCancel is to allow the underlying asynchronous operation (e.g. an Ajax request) to be cancelled. The minimal implementation above doesn't seem to have that ability, making it pretty useless I would think.

[skaegi]

Here's an example of what I'm thinking

function makeCancellableRequest(url) {
  var xhr = new XMLHttpRequest();
  var xhrPromise = // hookup resolver to xhr
  xhrPromise.then(null,  function (error) {
    if (error.name === 'Cancel') {
      xhr.abort();
    };
  });
  return xhrPromise;
}

I'm not saying an implementation wouldn't provide onCancel as a convenience.

I'm beginning to write tests now and noticed for external behavior I could replace the need for onCancel with a `catchCancellation/handleCancellation' block. That made me wonder if this is something that we really need to stipulate in the specification.

[domenic]

Interesting... nice illustration.

[skaegi]

Here's a gist to my Cancel tests -- https://gist.github.com/skaegi/4736504 . I just put the contents in CancelTests.js next to https://github.com/promises-aplus/promises-tests/tree/master/lib/tests and ran.

One thing I found that is relevant to the spec is that the cancel call to the parent in a "then promise" must happen after the original cancel call returns. This has to be done for cases like...

var apromise = fulfilled().then(function() {return anotherpromise;})
apromise.cancel();

... otherwise anotherpromise does not get a chance to "assume" apromise before the cancel call happens because the then contents are not fulfilled until the next tick. The two last tests from that gist above exercise the fulfilled and rejected variation of that case.

For the minimal implementation that means that instead of:

var _cancel = this.cancel.bind(this);

one should use...

var thisCancel = this.cancel.bind(this);
var _cancel = function() {
  setTimeout(thisCancel, 0); // or nextTick etc.
};

[skaegi]

I've updated my tests (https://gist.github.com/skaegi/4736504) to include an additional case where there was more than one fulfilled and rejected link in the promise chain before getting to a pending promise. For example:

var promise = fulfilled().then(function() {
            return fulfilled();
        }).then(function() {
            return pending();
        }).then(assert.fail, function(reason) {
            assert.ok(reason.name==='Cancel');
            done();
        });

This helped show a problem in the _cancel implementation above where the thisCancel does nothing because the parent promise has already been settled. Instead the implementation needs to check for the situation where _cancel has been re-assigned in which case it should call it instead of the original.

var thisCancel = this.cancel.bind(this);
var propagateCancel = function() {
  setTimeout(function() {
    var cancel = _cancel === propagateCancel ? thisCancel : _cancel;
    cancel();
  }, 0);
};
var _cancel = propagateCancel;

[kaerus]

For whatever it is worth, this is how I currently abort() a promise.
Requires the use of promise.attach() from the originator i.e an ajax wrapper.

Promise.prototype.attach = function(handle) {
    /* Attaches a handle (typically) to the promiser.   */
    /* In an Ajax scenario this could be the xhr object */ 
    this.attached = handle;

    return this;
}

Promise.prototype.abort = function(message) {
    /* notifies the attached process */
    if(this.attached && typeof this.attached.abort === 'function') 
        this.attached.abort();

    this.reject(message);

    return this;
}

[kaerus]

And timeout triggers abort().

Promise.prototype.timeout = function(time,func) {
    /* null cancels timeout */
    if(time === null) {
        clearTimeout(this._timer);
        return this;
    } 

    /* fulfilled or rejected */
    if(this._state) return this;

    /* trigger abort on timeout */
    if(!func) func = function() {
        this.abort("timed out");
    }

    this._timer = setTimeout(func.bind(this),time);   

    return this;
}