Disposer is not called when promise is cancelled

[xmirya]

Bluebird version: 3.1.8 Node.js: 7.2.1

Consider the following example code:

const Promise = require('bluebird');
Promise.config({
    cancellation: true
});
const acqPromise = new Promise(function (resolve, reject) {
    setTimeout(resolve, 1000);
});
const disposer = acqPromise.disposer(function () {
    console.log('DISPOSER CALLED');
});
const promise = Promise.using(disposer, function () {
    return Promise.resolve();
});
promise.cancel();

DISPOSER CALLED is never printed in this example (and it is printed when promise.cancel() is not called).

A real-world example of this behavior impact is Knex.js, a database access library that uses Bluebird promises; Disposer interface is used to acquire and release database connections from the pool, so when a query promise is created and canceled like this, the connection gets actually acquired and never released, draining the pool and hanging up the application.

[spion]

Promise.using(disposer, f) 

should desugar to

disposerPromise.then(f).finally(disposeFn)

and in this case, it looks like it doesn't.

[xmirya]

Some debugging: using.js has this:

    Disposer.prototype.resource = function () {
        if (this.promise().isFulfilled()) {
            return this.promise().value();
        }
        return NULL;
    };

The disposer is not invoked unless the resource acquiring promise has been fulfilled, and in the example provided it's not yet fulfilled, though it would in be one second, and then something has to release the resource.

[xmirya]

And it's not actually pending, it's cancelled - the resource allocator promise is cancelled when the main promise is cancelled. Which means that if resource allocator promise doesn't handle onCancel, it gets "forgotten" as 3.x cancellation semantics suggest. If this behavior is considered correct, whoever uses disposer interface must handle onCancel event in the allocator promise, otherwise there's no guarantee it would be disposed properly. Guess it's worth mentioning in the docs.

[spion]

@xmirya yeah I was originally going to suggest using onCancel, but on second thought, its better that the disposer is called even if the resource was not fully acquired, and the code provided by the user can hopefully handle that case too. Plus, that would mirror the obvious finally desugaring.

edit: I wonder, if the resource-acquiring promise doesn't provide a cancellation callback, perhaps it should not be cancellable from the perspective of the disposer?

[xmirya]

The problem i see is that resource passed to disposer is what acquiring promise returns. And if it's cancelled, it can't return anything despite the fact it might have been allocated (cancelling the acquiring promise itself seems like a sane idea to me). Thus both allocator and disposer need to track the resource via some external state, not managed by promises API.

[spion]

@xmirya isn't this already cancelling the acquiring promise? The problem is you cannot use disposer-using libraries that are not cancellation aware; they have to specifically use new Promise. And so the idea would be, "if the promise on which disposer was set up doesn't have an explicit onCancel callback defined, it will not be cancelled from the perspective of the disposer; instead, the disposer will wait for it to be fulfilled or rejected then it will run if there is a fulfilment value

[xmirya]

Mean it is already cancelling the acquiring promise and that's ok in case the acquiring promise is aware of onCancel, which i would just implement in my downstream code to make it work w/o any bluebird changes. But the solution looks good.

[petkaantonov]

The disposer cannot be called because there is no resource acquired by the promise.