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

promise.timeout() doesn't cancel the original promise after the timeout #891

Closed
spion opened this Issue Nov 25, 2015 · 15 comments

Comments

Projects
None yet
4 participants
@spion
Collaborator

spion commented Nov 25, 2015

With bluebird 2.x cancellable promises were being cancelled by timeout() however in 3.x that doen't happen even if cancellation is (globally) enabled. Is this intentional?

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Nov 29, 2015

Collaborator

I hope not.

Collaborator

benjamingr commented Nov 29, 2015

I hope not.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Nov 29, 2015

Collaborator

To reproduce:

bb2/cancel-bug.js

var Promise = require('bluebird')
var assert = require('assert')

function assertAfter(t, str) {
  return Promise.delay(t)
    .cancellable()
    .then(_ => console.error(str))
}

var p1 = assertAfter(1000, "Not cancelled");
var p2 = p1.timeout(500).catch(_ => _)

vs

bb3/cancel-bug.js

var Promise = require('bluebird')
var assert = require('assert')

Promise.config({cancellation: true})

function assertAfter(t, str) {
  return Promise.delay(t)
    .then(_ => console.error(str))
}

var p1 = assertAfter(1000, "Not cancelled");
var p2 = p1.timeout(500).catch(_ => _)

node bb2/cancel-bug has no output, while node bb3/cancel-bug prints:

Not cancelled
Collaborator

spion commented Nov 29, 2015

To reproduce:

bb2/cancel-bug.js

var Promise = require('bluebird')
var assert = require('assert')

function assertAfter(t, str) {
  return Promise.delay(t)
    .cancellable()
    .then(_ => console.error(str))
}

var p1 = assertAfter(1000, "Not cancelled");
var p2 = p1.timeout(500).catch(_ => _)

vs

bb3/cancel-bug.js

var Promise = require('bluebird')
var assert = require('assert')

Promise.config({cancellation: true})

function assertAfter(t, str) {
  return Promise.delay(t)
    .then(_ => console.error(str))
}

var p1 = assertAfter(1000, "Not cancelled");
var p2 = p1.timeout(500).catch(_ => _)

node bb2/cancel-bug has no output, while node bb3/cancel-bug prints:

Not cancelled
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 30, 2015

Owner

Cancellation is not rejection anymore, so how would it even work?

Owner

petkaantonov commented Nov 30, 2015

Cancellation is not rejection anymore, so how would it even work?

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Nov 30, 2015

Contributor

@petkaantonov: cancelling p1 should prevent the _ => console.error(…) callback from occurring and cancel the Promise.delay promise, shouldn't it?

Contributor

bergus commented Nov 30, 2015

@petkaantonov: cancelling p1 should prevent the _ => console.error(…) callback from occurring and cancel the Promise.delay promise, shouldn't it?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 30, 2015

Owner

what does that have to do with timeout whose purpose is to reject with a TimeoutError?

Owner

petkaantonov commented Nov 30, 2015

what does that have to do with timeout whose purpose is to reject with a TimeoutError?

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Nov 30, 2015

Collaborator

@petkaantonov What I meant is that .timeout() used to not only reject things, but called .cancel() on the promise. Not sure if the current .timeout() does that anymore, but I'll look into it and maybe send a PR of what I expect the behaviour to be.

Collaborator

spion commented Nov 30, 2015

@petkaantonov What I meant is that .timeout() used to not only reject things, but called .cancel() on the promise. Not sure if the current .timeout() does that anymore, but I'll look into it and maybe send a PR of what I expect the behaviour to be.

spion added a commit to spion/bluebird that referenced this issue Nov 30, 2015

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Nov 30, 2015

Collaborator

In the PR above:

  • derived promise is rejected
  • parent (original) promise is cancelled (propagating further up the tree)

Don't know if it makes sense to do it now as it may be considered a breaking change. Alternatively it may be considered a bugfix 😀 . I think its closer to BB2 behaviour (which called .cancel() on the promise).

Is this in conflict with the new cancellation semantics? I can think of one potential problem - what if multiple promises are derived from the parent, would that result with unexpected cancellation? If so, that should be fixable by deriving two promises (parent = ret.then(), child = parent.then()), returning the child, but cancelling the "parent"

Collaborator

spion commented Nov 30, 2015

In the PR above:

  • derived promise is rejected
  • parent (original) promise is cancelled (propagating further up the tree)

Don't know if it makes sense to do it now as it may be considered a breaking change. Alternatively it may be considered a bugfix 😀 . I think its closer to BB2 behaviour (which called .cancel() on the promise).

Is this in conflict with the new cancellation semantics? I can think of one potential problem - what if multiple promises are derived from the parent, would that result with unexpected cancellation? If so, that should be fixable by deriving two promises (parent = ret.then(), child = parent.then()), returning the child, but cancelling the "parent"

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Nov 30, 2015

Collaborator

Example use case (assumes cancellation enabled)

function whilist(condition, body, val = undefined) {
  return Promise.resolve(condition())
    .then(isTrue => !isTrue ? val : body().then(val => whilist(condition, body, val))
}

var waitButton = whilist(_ => $('.button').length === 0, () => Promise.delay(100))
waitButton.timeout(10000, "Page failed to load"); // should end `whilist` loop
Collaborator

spion commented Nov 30, 2015

Example use case (assumes cancellation enabled)

function whilist(condition, body, val = undefined) {
  return Promise.resolve(condition())
    .then(isTrue => !isTrue ? val : body().then(val => whilist(condition, body, val))
}

var waitButton = whilist(_ => $('.button').length === 0, () => Promise.delay(100))
waitButton.timeout(10000, "Page failed to load"); // should end `whilist` loop
@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Nov 30, 2015

Contributor

Is this in conflict with the new cancellation semantics?

No, I'd say it's totally in line with the new cancellation semantics. One might even consider cancelling with the TimeoutError instead of a CancellationError.

The only thing I'm not so sure about is whether parent.cancel() (in the PR) is enough. Conceptionally, you'd have to unsubscribe the handlers on parent (which were supposed to resolve the timeout promise that is now rejected) to make the cancellation work.

Contributor

bergus commented Nov 30, 2015

Is this in conflict with the new cancellation semantics?

No, I'd say it's totally in line with the new cancellation semantics. One might even consider cancelling with the TimeoutError instead of a CancellationError.

The only thing I'm not so sure about is whether parent.cancel() (in the PR) is enough. Conceptionally, you'd have to unsubscribe the handlers on parent (which were supposed to resolve the timeout promise that is now rejected) to make the cancellation work.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 30, 2015

Owner

No, I'd say it's totally in line with the new cancellation semantics. One might even consider cancelling with the TimeoutError instead of a CancellationError.

You cannot cancel with anything, cancellation is not rejection anymore.

Owner

petkaantonov commented Nov 30, 2015

No, I'd say it's totally in line with the new cancellation semantics. One might even consider cancelling with the TimeoutError instead of a CancellationError.

You cannot cancel with anything, cancellation is not rejection anymore.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Nov 30, 2015

Collaborator

@petkaantonov one option is to reject the derived promise but also cancel the parent one. If cancelling the parent is not a good idea due to other promises being potentially derived from it, e.g.:

this should not propagate to cancel the logging because p2 and p3 prevent that (multiple derived promises that did not request/derive from a timeout):

var Promise = require('bluebird');
Promise.config({
  cancellation: true
});

var p1 = Promise.delay(100)
  .then(_ => console.log("p1"))

var p2 = p1.delay(200).then(_ => console.log("p2"))
var p3 = p1.delay(300).then(_ => console.log("p3"))

p1.timeout(50) 

this should propagate to cancel the console.log and reject both p2 and p3 (only one derived promise that was "cancelled" when it timed out)

var Promise = require('bluebird');
Promise.config({
  cancellation: true
});

var p1 = Promise.delay(100)
  .then(_ => console.log("p1")).timeout(50)

var p2 = p1.delay(200).then(_ => console.log("p2"))
var p3 = p1.delay(300).then(_ => console.log("p3"))

then creating a parent/child pair within timeout then returning the child (cancelling the parent, rejecting the child) should fix that (this is not yet in the PR though)

Collaborator

spion commented Nov 30, 2015

@petkaantonov one option is to reject the derived promise but also cancel the parent one. If cancelling the parent is not a good idea due to other promises being potentially derived from it, e.g.:

this should not propagate to cancel the logging because p2 and p3 prevent that (multiple derived promises that did not request/derive from a timeout):

var Promise = require('bluebird');
Promise.config({
  cancellation: true
});

var p1 = Promise.delay(100)
  .then(_ => console.log("p1"))

var p2 = p1.delay(200).then(_ => console.log("p2"))
var p3 = p1.delay(300).then(_ => console.log("p3"))

p1.timeout(50) 

this should propagate to cancel the console.log and reject both p2 and p3 (only one derived promise that was "cancelled" when it timed out)

var Promise = require('bluebird');
Promise.config({
  cancellation: true
});

var p1 = Promise.delay(100)
  .then(_ => console.log("p1")).timeout(50)

var p2 = p1.delay(200).then(_ => console.log("p2"))
var p3 = p1.delay(300).then(_ => console.log("p3"))

then creating a parent/child pair within timeout then returning the child (cancelling the parent, rejecting the child) should fix that (this is not yet in the PR though)

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Dec 1, 2015

Contributor

@petkaantonov

You cannot cancel with anything, cancellation is not rejection anymore.

Well, it is to those callbacks that still observe cancellation, isn't it? I mean things like onCancel handlers, finally, and late subscribers. And I would think it's ok if those see a TimeoutError instead of a CancellationError.

@spion

If cancelling the parent is not a good idea due to other promises being potentially derived from it

That's fine, cancel() should automatically handle that.

Contributor

bergus commented Dec 1, 2015

@petkaantonov

You cannot cancel with anything, cancellation is not rejection anymore.

Well, it is to those callbacks that still observe cancellation, isn't it? I mean things like onCancel handlers, finally, and late subscribers. And I would think it's ok if those see a TimeoutError instead of a CancellationError.

@spion

If cancelling the parent is not a good idea due to other promises being potentially derived from it

That's fine, cancel() should automatically handle that.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Dec 1, 2015

Collaborator

@bergus it doesn't because the original promise gets cancelled, and therefore so will the other derived promises. Which is why I suggested creating a parent and a child (or rather, a child and a grand-child), cancelling the child and rejecting the grandchild.

Collaborator

spion commented Dec 1, 2015

@bergus it doesn't because the original promise gets cancelled, and therefore so will the other derived promises. Which is why I suggested creating a parent and a child (or rather, a child and a grand-child), cancelling the child and rejecting the grandchild.

spion added a commit to spion/bluebird that referenced this issue Dec 1, 2015

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Dec 1, 2015

Collaborator

Updated PR to address last point.

Collaborator

spion commented Dec 1, 2015

Updated PR to address last point.

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Dec 1, 2015

Contributor

Ah, I see, that's what parent = this.then() is good for.

Contributor

bergus commented Dec 1, 2015

Ah, I see, that's what parent = this.then() is good for.

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