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

Canceling a reduction #166

Closed
verma opened this Issue Mar 27, 2014 · 7 comments

Comments

Projects
None yet
2 participants
@verma

verma commented Mar 27, 2014

What would be the general strategy for canceling a Promise.reduce call? The promise itself cancels but the the current promise being waited upon for fulfillment is not notified of this cancellation.

E.g. how can I cancel this reduction?:

https://github.com/verma/plasio/blob/master/js/ui.js#L614

The fDataLoader and loadData methods are cancelable promises.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 28, 2014

Owner

The promise returned by Promise.reduce cannot be chained with the promises you return from the callback, so if you want to be able to cancel it, you can't use .reduce.

This should work however:

// do a progress function based on which file we're processing
var pfuncDataLoad = function(p, msg) {
    progress((currentLoadIndex + p*0.5) / maxLoadIndex, msg);
};

var pfuncDecompress = function(p, msg) {
    progress((currentLoadIndex + 0.5 + p*0.5) / maxLoadIndex, msg);
};

var soFar = [];
var cur = Promise.resolve().cancellable();

files.forEach(function(fname) {
    cur = cur.then(function() {
        return fDataLoader(fname, pfuncDataLoad).then(function(data){
            return loadData(data, pfuncDecompress);
        })
        .then(function(r) {
            var ret = {
                header: r[0],
                batcher: r[1]
            };

            // TODO: This needs to be fixed for mutliple URLs
            //
            ret.header.name = fname.name || name;
            currentLoadIndex ++;
            sofar.push(ret);
        })
    });
});

var loaderPromise = cur.then( ... )... rest of the code
Owner

petkaantonov commented Mar 28, 2014

The promise returned by Promise.reduce cannot be chained with the promises you return from the callback, so if you want to be able to cancel it, you can't use .reduce.

This should work however:

// do a progress function based on which file we're processing
var pfuncDataLoad = function(p, msg) {
    progress((currentLoadIndex + p*0.5) / maxLoadIndex, msg);
};

var pfuncDecompress = function(p, msg) {
    progress((currentLoadIndex + 0.5 + p*0.5) / maxLoadIndex, msg);
};

var soFar = [];
var cur = Promise.resolve().cancellable();

files.forEach(function(fname) {
    cur = cur.then(function() {
        return fDataLoader(fname, pfuncDataLoad).then(function(data){
            return loadData(data, pfuncDecompress);
        })
        .then(function(r) {
            var ret = {
                header: r[0],
                batcher: r[1]
            };

            // TODO: This needs to be fixed for mutliple URLs
            //
            ret.header.name = fname.name || name;
            currentLoadIndex ++;
            sofar.push(ret);
        })
    });
});

var loaderPromise = cur.then( ... )... rest of the code
@verma

This comment has been minimized.

Show comment
Hide comment
@verma

verma Mar 28, 2014

Thanks for your response and help.

I tried your solution every which way, the weird thing is that it works sometimes while other times it doesn't. The Promise.cancel is invoked but it doesn't go anywhere. I mean I don't see it being caught or raising an error anywhere.

Could be something funky I am doing. I am also uncertain on how to help you reproduce this issue.

verma commented Mar 28, 2014

Thanks for your response and help.

I tried your solution every which way, the weird thing is that it works sometimes while other times it doesn't. The Promise.cancel is invoked but it doesn't go anywhere. I mean I don't see it being caught or raising an error anywhere.

Could be something funky I am doing. I am also uncertain on how to help you reproduce this issue.

@verma

This comment has been minimized.

Show comment
Hide comment
@verma

verma Mar 29, 2014

So I took a closer look at the cancellation logic.

When we do Promise.resolve().cancellable(); we create a promise which is cancelable and already resolved. So at the top of the chain this.isCancelable() returns false since the promise is already resolved.

Promise.prototype.isCancellable = function Promise$isCancellable() {
    return !this.isResolved() &&
        this._cancellable();
};

At each level, to branch up to the parent, the code does this:

if (!this.isCancellable()) return this;
var parent;
if ((parent = this._cancellationParent) !== void 0) {
    parent.cancel(SYNC_TOKEN);
    return;
}

https://github.com/petkaantonov/bluebird/blob/master/js/browser/bluebird.js#L309

I feel that the top node (lets say C0) and the one below it (C1) have a little bit of a problem with this. C1 checks if its _cancellationParent is not null (which it isn't) and branches into the condition, C0.cancel(...) returns right away since C0.isCancellable() is false (since it is already resolved), which causes the control to come back to C1, which returns right away as well. This patterns repeats all the way to the bottom and promise fails to cancel (i.e. call _rejectUncheched function).

Still investigating, just wanted to update you. I am not entirely sure what's going on, I have very little knowledge of the whole thing.

Seems to me like the the C1 node here should have had its _cancellationParent set to null.

verma commented Mar 29, 2014

So I took a closer look at the cancellation logic.

When we do Promise.resolve().cancellable(); we create a promise which is cancelable and already resolved. So at the top of the chain this.isCancelable() returns false since the promise is already resolved.

Promise.prototype.isCancellable = function Promise$isCancellable() {
    return !this.isResolved() &&
        this._cancellable();
};

At each level, to branch up to the parent, the code does this:

if (!this.isCancellable()) return this;
var parent;
if ((parent = this._cancellationParent) !== void 0) {
    parent.cancel(SYNC_TOKEN);
    return;
}

https://github.com/petkaantonov/bluebird/blob/master/js/browser/bluebird.js#L309

I feel that the top node (lets say C0) and the one below it (C1) have a little bit of a problem with this. C1 checks if its _cancellationParent is not null (which it isn't) and branches into the condition, C0.cancel(...) returns right away since C0.isCancellable() is false (since it is already resolved), which causes the control to come back to C1, which returns right away as well. This patterns repeats all the way to the bottom and promise fails to cancel (i.e. call _rejectUncheched function).

Still investigating, just wanted to update you. I am not entirely sure what's going on, I have very little knowledge of the whole thing.

Seems to me like the the C1 node here should have had its _cancellationParent set to null.

@verma verma closed this Mar 29, 2014

@verma verma reopened this Mar 29, 2014

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 29, 2014

Owner

You are right, can you check if b3a5a0d works for you as well?

Owner

petkaantonov commented Mar 29, 2014

You are right, can you check if b3a5a0d works for you as well?

petkaantonov added a commit that referenced this issue Mar 29, 2014

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 29, 2014

Owner

Err I mean d818ff3

Owner

petkaantonov commented Mar 29, 2014

Err I mean d818ff3

@verma

This comment has been minimized.

Show comment
Hide comment
@verma

verma Mar 29, 2014

Yes, seems to be working great! Thanks!

verma commented Mar 29, 2014

Yes, seems to be working great! Thanks!

@verma verma referenced this issue Mar 29, 2014

Closed

Cancel not working #35

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 29, 2014

Owner

It is now included in the version 1.2 release

Owner

petkaantonov commented Mar 29, 2014

It is now included in the version 1.2 release

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