Skip to content
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

Stream created from a promise cannot handle cancellation. #486

Closed
jaroslav-muller opened this issue Apr 20, 2016 · 2 comments
Closed

Stream created from a promise cannot handle cancellation. #486

jaroslav-muller opened this issue Apr 20, 2016 · 2 comments

Comments

@jaroslav-muller
Copy link

I understand promise cancellation is not standardized, however I use bluebird together with highland and cancellation. Stream created via _(promise) gets blocked when the promise is cancelled.

It's caused by the fact that cancelled promise calls neither success nor error handler, so highland doesn't notice that the promise is finished.

However, it still calls finally handler. The fix seems to be easy, just change this:

function promiseStream(promise) {
    return _(function (push) {
        promise.then(function (value) {
                push(null, value);
                return push(null, nil);
            },
            function (err) {
                push(err);
                return push(null, nil);
            });
    });
}

to this

function promiseStream(promise) {
    return _(function (push) {
        promise.then(function (value) {
                push(null, value);
            },
            function (err) {
                push(err);
            }).finally(function() {
                return push(null, nil);
            });
    });
}

@vqvu
Copy link
Collaborator

vqvu commented Apr 20, 2016

finally isn't standardized either, so we'd need to gate the new behavior behind a _.isFunction(promise.finally) check. Do you mind submitting a PR for this?

@jaroslav-muller
Copy link
Author

I didn't realize finally is non-standard too.
Anyways, I created the PR. #487

@vqvu vqvu closed this as completed in 53a497e Apr 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants