Don't use a third "cancelled" state

[bergus]

(please tag 3.0)

It seems that #415 established that there should be no extra "cancelled" state. However, the current implementation seems to do just that - it settles the promise, and propagates this downstream, calling only onCancel and finally handlers but no onFulfilled or onRejected ones. E.g. in the test case described in #564, where a then callback returns a cancelled promise that is subsequently adopted and triggers a downstream onCancel.

For me, a cancellation should only propagate upstream, where it prevents fulfillment/rejection callbacks from being invoked, stops ongoing tasks and cleans up resources (by triggering onCancel and finally callbacks).

var p = Promise.delay(50).onCancel(expectCall()).then(expectNoCall(), expectNoCall()).finally(expectCall());
Promise.delay(25).then(() => { p.cancel(); assert(expectations); });

The cancellation should not propagate downstream. Instead, rejection with a CancellationError (for those who peek, usually .cancel() is called at the end of the chain anyway) seems appropriate here.

p.then(expectNoCall, expectCall()).onCancel(expectNoCall()); // regardless whether they are directly chained
Promise.delay(50).then(() => p.then(expectNoCall, expectCall()).onCancel(expectNoCall()) ); // or come later
Promise.delay(75).then(() => { assert(expectations); });

[Artazor]

In fact this sounds reasonable especially together with cancel request counting on branching.

[Artazor]

I've finally undersood what @bergus is proposing.

All pending upstream promises (where --p.subscribers === 0) should be sealed as rejected with CancellationError, however without running their onReject handlers. Moreover, they should share the same CancellationError instance as reason().

Instead of running onReject/catch handlers they should run their onCancel handlers (synchronously with initial .cancel()) - in natural order, not an inverse (like it is implemented at the moment). All exceptions that are thrown in onCancel handlers should be rethrown globally (process panic).

After all onCancel were executed .finally handlers should be executed (to clean up the resources) down to the cancellation point. .finally handlers on this way are executed according their usual semantics (including postponing on promises) except that errors/rejections should be rethrown globally (process panic).

After reaching the cancellation point, it should be turned into normal rejection with the same CancellationError for the downstream.

So here we see effective

• forget semantics;
• correct branching with cancellation;
• predictable fulfilment/rejection;
• no third cancellation state (and thus no promise.break).

Am I right @bergus? Also, want to hear again from @spion

[Artazor]

UPD: updated previous comment.

@petkaantonov what will be your verdict?

I personaly starting to believe that the described behaviour is better and more correct that the implemented one (3.0)

(sorry!)

[Artazor]

@petkaantonov, once you have found the courage to abandon the chosen design. Maybe now is the case when it worth to do it again?

Proposed changes will not affect those who use cancellation properly

• always use single consumer,
• don't store references to any intermediate promises
• cancel only the last promise in the chain (if cancel at all)

All othets are in potential trouble with the current design. And what proposes @bergus, looks like good point to stop and rethink/discuss it twice before publishing 3.0 as the proposed semantics is correct (IMO).

Anyway, Happy Easter :)

[petkaantonov]

onCancel doesn't propagate downstream anymore, this is needed for some internals but I made it possible to distinguish between internal onCancel hooks and user hooks

[petkaantonov]

It is true that one could potentially observe a cancelled state by doing something like:

var thenCalled = false;

.then(function() {
    thenCalled = true;
}, function() {
    thenCalled = true;
}).finally(function() {
    if (!thenCalled) {
        // Got here because of cancellation
    }
});

And of course with .reflect() as well (which gives pending: true, rejected: false, fulfilled: false)

However doing these should feel wrong enough (at least for the first case, .reflect is just reflecting) that people are not encouraged to react to cancellation as third state downstream.

[Artazor]

@petkaantonov I think that non-propogating onCancel to the downstream without rejecting the downstream breaks both designs.

Like @bergus I think that forget semantics should apply only on the path between cancellation point up to the farthest parent with single subscriber. On that way all promises even 'reflect' ones should be internally sealed as rejected for any subscribers that may be attatched to these promises later. The downstream should also should be treated as attatched later thus should be rejected.

Implemented cancellation now is infectious. If you touch it - you become infected with cancellation state. This is wrong. Cancellation should be atomic and non-discoverable after it had happened. All the mentioned upstream should be settled as rejected immediately and synchronously with .cancel().

If you chose not to propogate onCancel downstream only, without rejecting the downstream and leaving all the graph in the infectious state then the state of the program becomes inconsistent. I will provide you a contra-example for this (on the evening - i'm on the trip and writing from my phone).

@bergus - where are you? correct me if I'm wrong

[spion]

Looks okay to me. But I do have a question. What should I write in order to make db.transaction work with 3.0?

The API is:

db.transaction(tx => tx.query(...).then(_ => tx.query(...)))

and the current implementation is:

function transaction(f) {
  Promise.try(connectionPool.begin)
    .then(tx => Promise.try(f, tx)
      .then(res => tx.commitAsync().thenReturn(res),
            err => tx.rollbackAsync().catch(ignore).thenThrow(err)))
}

If I understand the new cancellation correctly, now I just need one more handler to handle the case where the promise created by f was cancelled?

Promise.try(f, tx)
      .onCancel(_ => tx.rollbackAsync().catch(ignore))
      .then(res => tx.commitAsync().thenReturn(res),
            err => tx.rollbackAsync().catch(ignore).thenThrow(err))

and this will correctly clean up the transaction in every single case?

But if the propagation is upstream only, it doesn't seem like the transaction will get cleaned up at all?

[petkaantonov]

@spion Cancellation is like long stack traces, it needs to be enabled before any promises are created in configuration. Since you don't use it you don't need to handle it at all.

[spion]

@petkaantonov this is a library function - what if a user of my library enables cancellation on their copy of bluebird for their needs?

[petkaantonov]

Then you need to use .using or use .reflect and do manually what .using is already doing

[spion]

Can I just add a finally handler?

Promise.try(f, tx)
      .then(res => tx.commitAsync().thenReturn(res),
            err => tx.rollbackAsync().catch(ignore).thenThrow(err))
      .finally(_ => tx.isOpen() && tx.rollbackAsync())

[petkaantonov]

That works too, finally is always called

[bergus]

Yes @Artazor I'm glad you understood me :-) I especially like your formulation

forget semantics should apply only on the path between cancellation point up to the farthest parent with single subscriber.

Exactly what I had in mind, but could not get into words… "Non-infectious" cancellation and "sealing cancelled promises as rejected for other downstream subscribers" sound good as well.

Happy easter!

[bergus]

I'm not sure about the order of cleanup/cancellation callbacks, though. Assuming

var p = promise().onCancel(a).finally(b).onCancel(c).finally(d);
p.cancel();

is there any order the callbacks need to be called in? Do users expect a specific order? My own library just runs upstream on the chain and calls d() c() b() a(). I don't know whether there is a need to guarantee such things?

[petkaantonov]

@bergus c a b d: .onCancel isn't part of the chain at all (and the second onCancel shouldn't even be possible if it could be enforced), in fact I would replace it with third argument to constructor if it was feasible (bluebird is used primarily through promisified functions that already return promises).

[petkaantonov]

Ok I will make onCancel only available inside constructor since the room for misunderstanding is too big and cannot be non-hackily enforced with an external method

[bergus]

Ah I see what you mean, onCancel is only for stopping the "currently active" task (of which there is only one in a linear chain), and would as such only be used inside the constructor; responding to one cancellation attempt. I thought it could also be used for "observing cancellation" like what you did above only with concise syntax. Though that's hardly needed.

[petkaantonov]

It's also entirely optional optimization, nothing should be different when you don't actually .abort() an xhr or clearTimeout(). In a situation where you would constantly make new requests and cancel them, not calling abort would only make things slower, not different.

[Artazor]

@petkaantonov, the decision to expose onCancel in promise constructor is reasonable and logical since in most cases this is the right place to define the aborting logic. However, it may introduce unnecessary new Promise in the code (that in other cases would be treated as anti-pattern). For example how would you rewrite the following (purely fictional) example without .onCancel and new Promise:

function loadResource(resource) {

  var loader = new FancyLoader(resource.name, {
    loading: 'loading.mpg',
    aborting: 'aborting.mpg',
    ready: 'ready.jpg',
    failed: 'failed.jpg'
  });

  form.add(loader); // plays 'loading.mpg' to the end user (assuming initial state == 'loading')

  return requestWithAsyncCleanupLogic(resource.url).onCancel(function(){

    loader.setState('aborting');  // plays 'aborting.mpg' to the end user

  }).finally(function() {

    // the time passed between cancellation and this handler may be significant
    // since the request being cancelled has an asynchronous cleanup (promises in finallies)
    if (loader.isInState('aborting')) {
      form.remove(loader);
    }

  }).then(function(result) {

    loader.setState('ready');
    loader.setDataPreview(result);
    return result;

  }, function(reason) {

    loader.setState('failed');
    loader.setDataPreview(formatError(reason));
    throw reason;

  });
}

I just wanted to demonstrate that there are valid use cases for public onCancel. But you are right

the room for misunderstanding is too big

[petkaantonov]

@Artazor

It's not too bad:

return new Promise(function(resolve, _, onCancel) {
    resolve(requestWithAsyncCleanupLogic(resource.url));
    onCancel(function(){
        loader.setState('aborting');  // plays 'aborting.mpg' to the end user
    });
})

Instead of

return requestWithAsyncCleanupLogic(resource.url).onCancel(function(){
    loader.setState('aborting');  // plays 'aborting.mpg' to the end user
});

Of course most people will probably manually wire up the reject instead of taking advantage of resolve

[Artazor]

Or the mentioned example is exactly the case you wanted to prevent? Was it prohibited to use .onCancel to expose the fact of cancellation to the outer world, and the only legitimate use for .onCancel was

entirely optional optimization

that should be non-observable except via the performance change?

[Artazor]

Oh, I've just seen your comment

[Artazor]

Of course, most people will probably manually wire up the reject instead of taking advantage of resolve

And this will be catastrophic for them since the following code

return new Promise(function(resolve, reject, onCancel) {
    requestWithAsyncCleanupLogic(resource.url).then(resolve, reject);
    onCancel(function(){
        loader.setState('aborting');  // plays 'aborting.mpg' to the end user
    });
})

is definitely an antipattern and is not equivalent to your example. Besides it will break the cancellation propagation to the upstream. Won't it?

[petkaantonov]

@Artazor yes if requestWithAsyncCleanupLogic had onCancel as well, it wouldn't be called

[Artazor]

The correct version (alas! still smells like an anti-pattern) of manual wiring up all handlers will be:

return new Promise(function(resolve, reject, onCancel) {
    var p = requestWithAsyncCleanupLogic(resource.url).then(resolve, reject);
    onCancel(function() {
        loader.setState('aborting');  // plays 'aborting.mpg' to the end user
        p.cancel();
    });
});

[Artazor]

@petkaantonov

_{I'm apologising for my English as I'm really not confident with my ability to express long thoughts in it. May be in other circumstances it would be better to meet in a video/chat, but my verbal English is even worse than written, and I don't expect that it will be comfortable to you.}

From the clash of opinions emerges the truth. I'll try to shatter your confidence in chosen design once more with the following sequence of observations:

1. We are implementing token-less cancellation, where .cancel() is performed directly on the promise we own.
2. There are two poles in promise usage patterns in the world (and both are legitimate):
   • single consumer usage -- a kind of structured programming for promises. Here all once created branches should be eventually merged back by all/race/join/map/etc.
     • You are allowed to store a reference to the tip of the chain only (no mid-chain references);
     • if new subscriber is attached to the tip, then you should update your reference to the newly created subscriber as a new tip;
     • the notion of the current tip becomes irrelevant during the branching until all created branches are merged back by some aggregation mechanism.
   • orchestrated usage -- a low level ability to create custom concurrent execution graphs. Though this approach is usually hidden by some higher level abstractions, it still uses branching/joining under the hood and may expose any intermediate promises according the abstraction rules.
3. The infectious third state definitely does not harm the idiomatic single consumer case. You can "consistently" cancel a tip p of the chain since both conditions are met:
   1. tip condition -- there are no pending subscribers of the p.
   2. balancing condition -- there are
      • either no pending ancestors of p with more than one subscriber
      • or every branching is balanced with corresponding merging before p;
4. Looks like that you are advocating only the idiomatic single consumer case, since it is sufficient for implementing all possible logical schemes like structured programming is capable to implement all possible algorithms. After structured programming has been proclaimed in 1968 as a silver bullet in a fight with algorithmization complexity we've got special syntax support in languages. Goto-less if and while. The famous Dijkstra's "Goto considered harmful" can be translated into "Promise branching considered harmful" (are you feeling yourself right enough to claim this loudly?). And the corresponding syntax support for structured asynchronous programming is definitely the async/await that enforces single consumer style (however not completely as you are free to deal with asynchronous values not awaiting on them, for example, to compose them via Promise.all())
5. When @bergus noted that cancellation might be unexpected for the parallel branch you've considered one possible solution: what's wrong with requiring a .cancel() from all branched consumers?. This solution does not harm the idiomatic single consumer, however, it is infeasible if all cancellations treated equally (i.e. we do not distinguish Expected/Unexpected cancellations). Infeasibility of this solution comes from subscribers that wanted to consume the result of the promise but are too late to prevent cancelling. For them, the cancellation is obviously unexpected. The only expected cancellations are for those promises that are located at and above the cancellation point. Anyway, I wanted to point out that there was a sparkle in your minds that cancel request counting could make things better. It definitely could, but only if we make Expected and Unexpected cancellations distinct. Let's dive into the philosophical nature of both.

6. Expected cancellation in some previous designs corresponds to the following pattern for onRejected handler:

   function(reason) {
      if (reason instanceof CancellationError) {
            // perform here any request aborts, etc.  (A)
            throw e; // always rethrow                (B)
      } else {
          // handle actual errors;                    (C)
      }
   }

   the inability to enforce this pattern in the userland yielded the 'Forget' semantics to be considered. The main idea here was to factor out A and B parts from the user's onReject handler and provide an ability to invoke A synchronously with .cancel(). Here C is unaware of any exception. It can be illustrated with the following scheme:

_to be continued..._ Just wanted to publish what I've already typed. There will be more points, illustrations and comments tomorrow. (Sorry)

[spion]

@Artazor this is great, I eagerly await your updates/continuation :)

edit: you might want to publish this as a blog post as well. By the way your English is excellent and it seems to me that with a few extra passes at the piece you will arrive at a nicely polished article on cancellation

[petkaantonov]

So for multi branched promises, a promise should have its onCancel called once all its branches have cancelled, but even if not every branch cancels, those branches that cancelled never get their callbacks called (except finally of course).

If on top of this, you need to somehow swoop in and attach a new branch to a promise that has been cancelled, the newly branched promise should just be rejected with cancellationerror?

And that's it? We have sane promise cancellation so easily?

[spion]

Just an idea: it would be nice to come up with 2 or 3 different use cases and see how different designs would apply to them.

[spion]

Here is a simple use case:

A series of two XHRs are sent to the server to fetch search results: the first one fetches IDs matching the search box text, the next one returns a dictionary of actual objects that correspond to those IDs. At the same time a spinner is activated to indicate that an operation is in-flight. There are two possible scenarios:

1. requests complete, then result or error is shown.
2. user cancells the operation by pressing Esc in the search box (If you want to make things harder, the page should indicate that the operation was cancelled by adding an "Operation cancelled" message to the page)

In both situations the spinner should disappear at the end.

How would the code look like in this design? More interestingly, how would it look on the back end? (cancellation is caused by the HTTP connection being closed; every operation looks like this: allocate transaction, then perform a series of queries within it)

[Artazor]

@spion - yes, I've planned to demonstrate pros and cons of different designs on several concise yet realistic use cases. Just need more time to prepare the materials (I'll include your example)

@petkaantonov

And that's it? We have sane promise cancellation so easily?

roughly - yes. In details - no. Anyway, there is also a room for uncertainty. I'll try to cover all my considerations today in the late evening (after ~23:00 EEST)/tonight.

The design that I'm investigating at the moment involves the following:

• Extension to .cancel() -- cancel([Promise target]) here we define an optional target for the cancellation -- the ability to put a barrier on the path of the cancellation: cancellation should not be bubbled up above the target, even if there are pending ancestors of the target in upstream. This parameter emerges since we fused promises with cancellation tokens together. Thus the promise instance itself can be used as a reference point in the cancellation chain. I believe that in any design the ability to define a barrier for the cancellation will help to make things more structured (in a spirit of structured programming). -- I will draw a graphical explanation later. [it implies that onCancel registered functions will accept the target as well]
• The cancellation itself is splitted into following phases:
  1. Synchronous sealing all the graph between cancellation point (source) and the cancellation target (farthest possible unless supplied) as rejected with CancellationError (done with simple in-depth-first graph traversal since there are no cycles) - there is no contradiction with not calling the onReject as we know it should contain only a "rethrow" logic for cancellation.
  2. Synchronous notifying all vertices sealed at the previous step by calling of .onCancel registered handlers
  3. Asynchronous finalising (with merging on aggregates if there was balanced branching/merging)
     • There should not be any UnhandledRejecteion if cancellation source (promise on which .cancel() has been invoked) after its finalising is done have no attached subscribers.

Maybe phases 1 and 2 can be performed in one pass.

• Other (orthogonal) extension to .cancel() -- .cancelAsync(): Promise that returns a promise that fulfills when the cancellation source has been finalized.

_{Arrrrgh, without simple graphical representation it is too hard to explain it by words}

[bergus]

@Artazor thanks for the comprehensive reading _{(and your English is definitely no worse than mine!)}. I'm unsure about the terminology for "single consumer case", though. In this, a promise on its own could still have multiple consumers, couldn't it? The "branches are merged back" case is quite common imo. You relate this to structured programming, I would think a "single exit point" matches this better "branching is harmful". But actually none of this describes parallel execution well (where we want to spread the flow), so I'd rather use the phrase open (promise chain) ends considered harmful.

@petkaantonov Yes, that's it :-) So sane, so simple! That was the idea in my draft, but I fear I failed to make the point clear enough. I was pretty much focused on those "cancellation tokens" which I used to describe (and implement)

if not every branch cancels, those branches that cancelled never get their callbacks called (except finally of course).

but I guess those tokens have confused everyone who was comfortable with the .net CancellationTokenSource model.

If Bluebird uses this in 3.0, I'm looking forward to see how you implement the ref-counting and callback-cancelling efficiently. Also, I'd love to get this behaviour constituted in a cancellation specification that also provides a way to make cancellation interoperable (in the thenable assimilation procedure).

[petkaantonov]

If Bluebird uses this in 3.0, I'm looking forward to see how you implement the ref-counting and callback-cancelling efficiently

Well, I don't :-D. Cancellation is already behind a switch because it takes 3 additional fields on the promise + new closure needs to be allocated for every new Promise. But you only pay for this if you actually use cancellation.

[bergus]

Oh I didn't see your latests posts. @Artazor

the ability to put a barrier on the path of the cancellation

I would not make this "barrier" an argument to the cancel method. After all, the canceller usually does not know whether/that there is a barrier he is supposed to respect. In my drafts, cancel takes an optional error argument (that defaults to a CancellationError) which is passed to all the rejections of uncancelled callbacks.

To implement a "barrier", I thought of a .uncancellable() method which returns a new promise that does not propagate cancellation attempts upstream (and is never cancelled itself). Of course, callbacks attached to it via .then would still be cancellable on their own.

This is based on an algorithm a bit different from yours: sealing and notifying are merged into one phase. A .cancel() does only notify the promise it was called on of the cancellation. The notified promise then can decide on its own what should happen - testing if cancellation should propagate further, and if so calling onCancel callbacks, sealing and notifying all the promises it depends on. The depth-first traversal of the DAG would be implied by this.

[Artazor]

@bergus - you are right about terminology. Of course, the branching itself is not harmful as soon as it balanced with merging back and providing that there eventually will be a single exit point (exactly like in structured programming). So I'll use your open (promise chain) ends considered harmful and the "single exit point" in upcoming comments/updates (as well as when/if our adventure will be documented as a blog post :)

About barrier - I will show the drawbacks of 'uncancellable()'. Anyway, I know at least three different approaches how to deal with that problem (hope to cover them as well).

[bergus]

@spion Here is what I would do:

function fetch(url, data) {
    return new Promise(function(resolve, reject) {
        var xhr = new XMLHttpRequest();
        xhr.open("get", url, true);
        xhr.onerror = reject;
        xhr.onload = function(e) { resolve(this.response); };
        xhr.responseType = "json";
        xhr.send(data);
        return function onCancel(err) {
            xhr.abort();
        };
    });
}
function next(target, type, predicate) {
    return new Promise(function(resolve) {
        function listener(e) {
            if (!predicate(e)) return;
            fin();
            resolve(e);
        }
        function fin(err) {
            target.removeEventListener(type, listener);
        }
        target.addEventListener(type, listener, false);
        return fin;
    });
}
var search = document.getElementById("searchbox");
search.addEventListener("change", function(e) {
    var esc = next(search, "keydown", functon(e) { return (e.keyCode || e.which) == 27; });
    var result = fetch("/search-ids", search.value).then(function(ids) {
        return fetch("/dictionaries", {ids: ids});
    });
    showSpinner();
    var display = result.finally(hideSpinner).then(showResults, showError);

    Promise.race(esc, display); // the first one cancels the other
    // alternatively:
    Promise.race(esc, result); // similar, but hitting esc invokes `showError`
    // alternatively:
    Promise.race(esc.then(showCancelMessage), display)
    // alternatively:
    var hasCancelled = esc.then(function() {
        result.cancel(new Error("message")); // invokes `showError` with custom argument
    });
    result.finally(function() { hasCancelled.cancel(); });
}, false);

Of course, cancellation is complicated, and not using race here (in the last case) introduces lots of chances for bugs. E.g. cancelling esc instead of hasCancelled - then esc and result might both resolve at the same time (without effects in above example, but unexpected nonetheless). Or using result.then instead of .finally - then in case of a rejection the event handler would leak.

[Artazor]

Guys, I'm sorry, seems that update will be in next two days!

[petkaantonov]

@Artazor any update? :p

[Artazor]

Sorry! I've tried to build a formal framework for reasonong about cancellation and seems dig too deep. Tonight I'll publish an update -)

[Artazor]

update is comming...

[spion]

@Artazor any news? :D

[Artazor]

@spion, you dont know how close to the truth this picture is. I hope I'll publish my investigation and the spec for cancellation on saturday/sunday. But yes, I've gone deeper and constructed other entity very close to Promises/A+ (in fact almost all specs hold but in generalised form) and it lacks almost all problems that come with promises, including possiblyUnhandled rejection (it is impossible to to miss the exception or it never really happenedhappened), and it has consistent cancellation, and other useful properties. So my cancellation semantics will be simple "downporting" to Promises/A+. By the way my "Promises" are more lightweight and scheduler independent (however all async-related formal properties are not violated). The only thing I'm afraid of that they are already invented.

-)

[benjamingr]

@Artazor just make sure you don't give up on anything fundamental promises are awesome at:

• broadcast
• eager
• cached\immutable
• chaining

;)

[bergus]

@benjamingr Maybe off-topic here, but what exactly is so awesome about eagerness?

[Artazor]

@bergus, are you clairvoyant? :D

In my model Thunks/A+ I have lazy '.thus' that is lazy analog of 'then' and 'then' in my model has role of 'done' which converts a chain into real eager promise. But you should not call 'then' often (just as done in bluebird). But yes, I think it is an offtopic.

[benjamingr]

Yup, definitely reinventing.