Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#18219 closed defect (worksforme)

Canceling Deferred.promise has inconsistent flow for chained promises when specifying error callback functions

Reported by: tanneman Owned by: tanneman
Priority: undecided Milestone: tbd
Component: Core Version: 1.9.3
Keywords: Cc:
Blocked By: Blocking:

Description

Canceling the promise will result in the chained promises to be rejected. However, this is only done when there is no error callback specified. When there is an error callback the chained promises are put on resolved. I have exposed the issue using the code below:

  • On a side note: I would expect the chained promises to be canceled as well. This is not the case, but at least consistent for either use case, and this is not my issue so I will ignore this.

Code to reproduce:

function cb(resp) {
   return resp;
}

/* Specifying NO error callback results in the chained promise being rejected */
function testScenario1() {
  var promise = new Deferred().promise;
  var chainedPromise1 = promise.then(cb);
  promise.cancel();

  assertTrue('promise should be canceled', promise.isCanceled());
  assertTrue('promise should be rejected', promise.isRejected());

  assertFalse('chained promise is not canceled (but should be)', chainedPromise1.isCanceled());
  assertTrue('chained promise is rejected', chainedPromise1.isRejected());
  assertFalse('chained promise is not resolved', chainedPromise1.isResolved());
}


/* specifying an error callback results in the chained promise being resolved */
function testScenario2() {
  var promise = new Deferred().promise;
  var chainedPromise1 = promise.then(cb, cb);
  promise.cancel();

  assertTrue('promise should be canceled', promise.isCanceled());
  assertTrue('promise should be rejected', promise.isRejected());

  assertFalse('chained promise is not canceled (but should be)', chainedPromise1.isCanceled());
  /* These asserts fails, the chainedPromise is resolved instead of rejected */
  assertTrue('chained promise is rejected', chainedPromise1.isRejected());
  assertFalse('chained promise is not resolved', chainedPromise1.isResolved());
}

Change History (4)

comment:1 Changed 5 years ago by ben hockey

Owner: set to tanneman
Status: newpending

is this the same issue reported in #18131? i see that you've put the version as 1.9.3, does master work as you would expect?

comment:2 in reply to:  1 Changed 5 years ago by tanneman

Status: pendingnew

Replying to neonstalwart:

is this the same issue reported in #18131? i see that you've put the version as 1.9.3, does master work as you would expect?

Last edited 5 years ago by tanneman (previous) (diff)

comment:3 Changed 5 years ago by ben hockey

Resolution: worksforme
Status: newclosed

with the isCanceled issue, it's debatable about what should happen and seems like it could be quite a long discussion if we try to change that behavior at this point, given how long it's been like this - if it were a regression on some previous behavior we would consider changing it. fortunately, you're ok ignoring this and personally i get by just fine without ever using any of the Promise#is* methods which provides anecdotal evidence that there are ways to avoid these methods if they aren't working how you'd like.

with regard to the rejection not working as you expect, this is actually working as it should.

as an analogy, think of the error callback as an asynchronous catch block. in a synchronous try/catch if you throw an error in the try block and then catch an error, the code following the try/catch proceeds as if there were no error - the chained callback is working this way when you provide an error handler to "catch" the asynchronous error from the promise.

continuing with the analogy, if you were to rethrow an error from the synchronous catch block, then the code following the catch block would not execute. with a promise if you rethrow the error from your error handler then the chained promise will be rejected too - http://jsfiddle.net/2dztxedy/5/

hopefully the analogy helps explain why it's working the way it does.

comment:4 in reply to:  3 Changed 5 years ago by tanneman

Thanks, your explanation does makes some sense.

Also, it seems if I want to propagate the reject handling then I should have the errorCallback throw it as well. That fixes the issue for me.

Note: See TracTickets for help on using tickets.