.done() signifies a promise is handled; chains that end in it are not runaway promises

[RoboPhred]

1. What version of bluebird is the issue happening on?
   3.5.1

2. What platform and version? (For example Node.js 0.12 or Google Chrome 32)
   NodeJS 8.9.1

3. Did this issue happen with earlier version of bluebird?
   Unknown - new usage.

Currently, there seems to be no way for a library that wishes to use promises but expose a callback api without causing floods of false-positive runaway promise warnings if a consuming library then wraps those callbacks in other bluebird promises.

It seems the semantics of .done() indicate that the promise is absolutely at an end and fully handled, so we have been using it for promises that perform setup work inside things like streams, which communicate their readiness over EventEmitter events rather than exporting a promise. As such, any promise chain in which all branches end in a .done() should not be considered a runaway promise error.

After a few minutes of experimentation, I was able to make a trial implementation of this in debuggability.js by calling this function from checkForgottenReturns against promiseCreated

function checkEventuallyFinal(promise) {
    // The promise is already final.
    if (promise._isFinal()) return true;

   // We are not final and no one chains us.  We are unhandled.
   if (promise._promise0 == null) return false;

    // If our chained promise is not eventually handled, neither are we.
    if (promise._promise0 && !checkEventuallyFinal(promise._promise0)) return false;

    // Check additional promises.
    var len = (promise._bitField & 65535);
    for(var i = 1; i < len; i++) {
        var child = promise._promiseAt(i);
        if (!checkEventuallyFinal(child)) return false;
    }

    // All of our chained promises are eventually handled.
    return true;
}

I have tested this out against our existing code, and it reliably silences the error when the promises are fully handled and end in .done(), and still warns when it detects 'true' runaway promises.

If performance is an issue, a bit field could be added indicating if all children are final, and propagated on each .done() call.

[benjamingr]

Thanks for raising the issue :)

First of all - .done is deprecated and should not be used

Currently, there seems to be no way for a library that wishes to use promises but expose a callback api without causing floods of false-positive runaway promise warnings if a consuming library then wraps those callbacks in other bluebird promises.

We have the .asCallback utility method for that:

myPromiseReturningFn().asCallback(cb); // cb is a standard node callback
function callbackApi(cb) {
  promiseReturningFn().asCallback(cb);
}

[RoboPhred]

Hmm, this was the first thing I tried, and I still got the same warnings. I ended up reasoning that asCallback did not mark the promise as final because it returns yet another promise itself.

I will investigate further.

[RoboPhred]

Is the semantics of asCallback() to forward responsibility to the callback passed in? If so, this should always suppress the error; the promise is going to be handled by the callback and bluebird can wash its hands of it.

At any rate, I am still seeing this warning with asCallback, so bluebird thinks the promise is not guaranteed to be handled.

Here is a distilled version of what a lot of our consuming code is doing that generates the warning:

const Promise = require("bluebird");
const EventEmitter = require("events");


function waitPromise() {
    return new Promise((accept) => {
        setTimeout(() => accept(), 1000);
    });
}


function createMyEmitterApi() {
    const emitter = new EventEmitter();

    // This does not emit the warning.
    const connectPromise = waitPromise().then(() => 42).asCallback((err, result) => {
        if (err) emitter.emit("error", err);
        else emitter.emit("connect");
        return null;
    });

    emitter.close = () => {
        // Calling emitter.close() and having a continuation will
        //  trigger the warning, despite the asCallback.
        connectPromise.then(c => {
            // Close logic here.
            return waitPromise();
        }).asCallback((err, result) => {
            if (err) emitter.emit("error", err);
            emitter.emit("close");
            return null;
        });
    };
    return emitter;
}


console.log("creating emitter");

const myConsumer = new Promise((accept, reject) => {
    const apiObj = new createMyEmitterApi();
    apiObj.once("connect", () => accept(apiObj));
});

function consumeThenClose() {
    return myConsumer.then(r => {
        console.log("We got the emitter");
        console.log("The consumer is now listening to the close event outside of the project chain.");
        r.on("close", consumerCloseHandler);

        return waitPromise(1000).then(() => r);
    }).then(r => {
        console.log("Closing emitter");
        // This is typically legal with our api, pre-bluebird.
        //  The close listener might be external to this promise chain.
        // close() does not return a promise; the consumer shouldn't even know
        //  that there are promises at play here.
        // The promise identified by the warning is fully handled with
        //  an asCallback on line 30, and the result is continued through
        //  the event emitter.
        r.close();

        // A return-null here would suppress the error, and is probably best practices anyway,
        //   but this promise is treated as a no-value object to wait on in the actual code,
        //   and this consumer should behave the same regardless of our lib's usage of
        //   promises
    });
}

// Attaching a promise continuation to the result of this function triggers the warning
//  against r.close() on line 63
// If we did not attach the .then(), it would not error, 
consumeThenClose().then(() => {
    console.log("consumeThenClose() completed.");
});


function consumerCloseHandler() {
    console.log("api consumer has seen the emitter close.");
}

I have an api that returns an EventEmitter. This EventEmitter uses bluebird for work in its constructor (which is continued by a "connect" event.), and for a .close() function (which emits a "close" event as the promise continuation).

These objects will often be created inside new Promise() call, which wires up a global close event handler and completes when the connect event is raised. This is rather unorthodox, but these items are long-lived and intended to run indefinitely. These promises are then stored in an array until the client receives a shutdown signal.
When signaled to close, the promises are waited on and .close() is called. However, it is up to the global close handler to handle these, since we may also get close events if the remote end closes us.

I would like to avoid requiring the clients to return null from their promises whenever calling one of our api functions. A lot of code already exists that would need to be changed, and the story of "fix it with return null" has so far ended up suppressing several legitimate runaway promises.

If asCallback is really contracted as marking the promise as handled, this is probably a bug. Otherwise, I feel that there should be some other way of claiming responsibility of the promise's continuation.
(It's a shame done is deprecated, the semantics of "this is the final handler" seems to naturally fit this behavior).