Warning: a promise was created in a handler but none were returned from it

[rayshan]

Hello, trying out the 3.0 branch. Have a question regarding this commit: https://github.com/petkaantonov/bluebird/commit/54b627d243556399c9b3f99555d2aaede34cb27c

Is it best practice to always return a promise created in a promise handler? What's the reasoning behind it?

[threehams]

I'm glad this was finally added. One of the most common mistakes when first using promises is something like this:

fs.readFileAsync('./stuff.txt').then(function(stuff) {
  requestAsync({url: 'http://localhost/users', method: 'POST'});
}).then(function() {
  // stuff you want to do only after the request is completed and successful
});

If you forget to return that request promise, the next handler executes immediately with an argument of undefined - which is completely valid for the Promises/A+ spec, but not what you want 99% of the time.

[petkaantonov]

What @threehams said, it's not about best practices but your code actually being broken. It is extremely unlikely that creating a promise inside handler but not returning it is what you want, because it creates a runaway promise that is not connected to any chain, therefore its errors cannot be handled and obviously the async operation cannot be awaited for by anything.

[rayshan]

Got it, makes a lot of sense. Thanks guys.

[aegyed91]

this warning will trigger where there is no search result?

User
      .findOne({ $or: $or })
      .exec()
      .then((user) => {

[petkaantonov]

@tsm91 huh? can you paste more complete example with the stack trace

[aegyed91]

ahh sorry, i thought a query with no results will trigger this warning message, but i tried it with different scenarios and this is not the case

at a specific case i am not returning the promise probably, but i could not figure it out where yet, i don't think so the problem is in passport.

here is my code and the stack trace:

// ## Facebook Authentication
  if (settings.facebook.enabled) {
    // web-based
    passport.use(new FacebookStrategy({
      callbackURL: settings.url + '/auth/facebook/callback',
      clientID: settings.facebook.appID,
      clientSecret: settings.facebook.appSecret
    }, providerAuthCallback)); // eslint-disable-line

    // token-based
    passport.use(new FacebookTokenStrategy({
      clientID: settings.facebook.appID,
      clientSecret: settings.facebook.appSecret
    }, providerAuthCallback)); // eslint-disable-line
  }

/**
   * A common oauth provider callback
   *
   * @param {String} accessToken  -
   * @param {String} refreshToken -
   * @param {Object} profile      - profile object with the requested information from the provider
   * @param {Function} done       - cb
   * @returns {*}
   */
  function providerAuthCallback(accessToken, refreshToken, profile, done) {
    if (
      profile.emails.length === 0
      || !_.isObject(profile.emails[0])
      || !validator.isEmail(profile.emails[0].value)
    ) {
      return done(new Error(util.format(LNG.NO_SOCIAL_EMAIL, profile.provider)));
    }

    let $or = [{ email: profile.emails[0].value }];

    // normalize the auth callbacks by simply pushing to the
    // $or query that will be executed with User.findOne below
    // this allows us to simply have one auth callback for
    // different providers like Facebook, Google, etc.
    let provider = {};
    provider[profile.provider + '_id'] = profile.id;
    // note that we unshift instead of push, since we want findOne
    // to return the user based off `profile.id` which takes
    // precedence over the user's email address in `profile.emails`
    $or.unshift(provider);

    User
      .findOne({ $or: $or })
      .exec()
      .then((user) => {
        if (user) {
          if (!user[profile.provider + '_id']) {
            user[profile.provider + '_id'] = profile.id;
          }

          if (accessToken) {
            user[profile.provider + '_access_token'] = accessToken;
          }

          if (refreshToken) {
            user[profile.provider + '_refresh_token'] = refreshToken;
          }

          return user.save(done);
        }

        let avatar;

        /* eslint-disable indent */
        switch (profile.provider) {
          case 'facebook':
            avatar = `https://graph.facebook.com/${profile.id}/picture`;
            break;
          case 'google':
            avatar = profile._json.image.url.split('?')[0];
            break;
          default:
            const hash = crypto.createHmac('sha1', settings.hashKey).update(profile.emails[0].value).digest('hex');
            avatar = `https://gravatar.com/avatar/${hash}?d=identicon`;
        }
        /* eslint-enable indent */

        let userInfo = {
          email: profile.emails[0].value,
          name: profile.displayName,
          gender: profile.gender || profile._json.gender,
          avatar: avatar,
          [profile.provider + '_id']: profile.id
        };

        if (accessToken) {
          userInfo[profile.provider + '_access_token'] = accessToken;
        }

        if (refreshToken) {
          userInfo[profile.provider + '_refresh_token'] = refreshToken;
        }

        // if the user signed in with another service
        // then create a random password for them
        User.register(userInfo, randomstring.token(), function(err, updatedUser) {
          if (err) {
            return done(err);
          }
          if (!updatedUser) {
            return done(new Error(LNG.SIGNUP_ERROR));
          }
          done(null, updatedUser);
          updatedUser.sendWelcomeEmail();
        });
      })
      .catch((err) => done(err));
  }

the stack trace:

Warning: a promise was created in a handler but none were returned from it
    at processImmediate [as _immediateCallback] (timers.js:368:17)
From previous event:
    at Strategy.providerAuthCallback [as _verify] (/Users/tsm/Sites/dd/etc/init/03-sessions.js:118:8)
    at /Users/tsm/Sites/dd/node_modules/passport-google-oauth/node_modules/passport-oauth/node_modules/passport-oauth2/lib/strategy.js:195:22
    at /Users/tsm/Sites/dd/node_modules/passport-google-oauth/lib/passport-google-oauth/oauth2.js:115:7
    at passBackControl (/Users/tsm/Sites/dd/node_modules/passport-google-oauth/node_modules/passport-oauth/node_modules/passport-oauth2/node_modules/oauth/lib/oauth2.js:125:9)
    at IncomingMessage.<anonymous> (/Users/tsm/Sites/dd/node_modules/passport-google-oauth/node_modules/passport-oauth/node_modules/passport-oauth2/node_modules/oauth/lib/oauth2.js:143:7)
    at emitNone (events.js:72:20)
    at IncomingMessage.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:903:12)
    at doNTCallback2 (node.js:439:9)
    at process._tickCallback (node.js:353:17)

[petkaantonov]

What is line 118?

Also mixing promises and callbacks like this is super confusing, you can just make normal promise chain and call .asCallback(done) at the end which will automatically map the promise chain properly to callback api.

[aegyed91]

line 118: .then((user) => {

yeah, this file is pretty much a mess, you mean like this?

return user
            .save()
            .asCallback(done);

[petkaantonov]

I mean doing:

User
  .findOne({ $or: $or })
  .exec()
  .then((user) => {
    if (...) {
        ....
        // Make sure it returns a promise
        return user.saveAsync();
    }

    ...
    // Make sure it returns a promise
    return User.registerAsync();
  })
  .asCallback(done)

Hold on, I will publish 3.0.4 which will makae the stack trace of forgotten return warning much more useful.

[petkaantonov]

Can you retry in 3.0.4 and see what the stack trace is now?

[aegyed91]

Warning: a promise was created in a handler but none were returned from it
    at Query.exec (/Users/tsm/Sites/dd/node_modules/mongoose/lib/query.js:2111:10)
    at Function.schema.statics.findByUsername (/Users/tsm/Sites/dd/node_modules/passport-local-mongoose/index.js:294:13)
    at Function.schema.statics.register (/Users/tsm/Sites/dd/node_modules/passport-local-mongoose/index.js:237:10)
    at /Users/tsm/Sites/dd/etc/init/03-sessions.js:171:14
    at processImmediate [as _immediateCallback] (timers.js:383:17)
From previous event:
    at Strategy.providerAuthCallback [as _verify] (/Users/tsm/Sites/dd/etc/init/03-sessions.js:118:8)
    at /Users/tsm/Sites/dd/node_modules/passport-oauth2/lib/strategy.js:195:22
    at /Users/tsm/Sites/dd/node_modules/passport-google-oauth/lib/passport-google-oauth/oauth2.js:115:7
    at passBackControl (/Users/tsm/Sites/dd/node_modules/oauth/lib/oauth2.js:125:9)
    at IncomingMessage.<anonymous> (/Users/tsm/Sites/dd/node_modules/oauth/lib/oauth2.js:143:7)
    at emitNone (events.js:72:20)
    at IncomingMessage.emit (events.js:166:7)
    at endReadableNT (_stream_readable.js:905:12)
    at doNTCallback2 (node.js:450:9)
    at process._tickCallback (node.js:364:17)

[petkaantonov]

Yea that will go away when you use the promise returned by User.register

[aegyed91]

The problem is there is no registerAsync function, this register method on user model is not a built in mongoose thing, this is from https://github.com/saintedlama/passport-local-mongoose/blob/master/index.js#L226-L258

i also promisified it in the user model

import passportLocalMongoose from 'passport-local-mongoose';
Promise.promisifyAll(passportLocalMongoose);

I also use mongoose.Promise = require('bluebird'); so i think mongoose model.save is okay, i dont need saveAsync.

but the main problem here is this register method

[petkaantonov]

Oh I see now. Mongoose is creating promises even when the callback api is used. And because you assigned mongoose.Promise = require("bluebird") they will create bluebird promises.

Instead of doing mongoose.Promise = require("bluebird") assignment, you should do Promise.promisifyAll(require("mongoose")) and use the Async suffixed methods.

[petkaantonov]

Alternatively you can also promisify the "passport-local-mongoose" statics and use return registerAsync

[aegyed91]

yes, so basically if you can do user.save(cb[err, user]) or return user.save().then(user).catch(err)

but is there any difference between promisify-ing the mongoose lib or use this feature as of mongoose 4.1.0 to be able to specify which promise library to use? https://github.com/Automattic/mongoose/issues/2688

[benjamingr]

@tsm91 meh: https://github.com/Automattic/mongoose/issues/2688#issuecomment-104422889

[petkaantonov]

You will not see any warnings that are out of your control if you promisify mongoose. If mongoose internally makes mistakes you will see warnings that you can't do anything about if you assign mongoose.Promise to bluebird.

[petkaantonov]

what do you mean by help? It gets rid of any warning such as this one which is out of his control

[aegyed91]

so basically i should drop this feature and do Promise.promisifyAll(require("mongoose")) .

it shouldnt be hard to append an Async to all affected methods, i hope i wont break anything

[benjamingr]

@petkaantonov the mongoose people are usually pretty responsive, maybe we should talk to them.

Stuff like https://github.com/Automattic/mongoose/blob/master/lib/query.js#L2101-L2127 is mostly wasteful. It's an interesting case anyway.

[petkaantonov]

Asking other libraries to change when they haven't really done anything wrong is not scalable and is pretty silly

[benjamingr]

Of course, but in this particular case - Mongoose only recently implemented their own promise support and they'd be interested in hearing about it. That's all I'm saying.

[aegyed91]

Meh, for now i will use mongoose.Promise = require('bluebird') I have more than 50 queries.

Anyways i just finished fixing my third bug where i don't return a promise in the chain thanks to this warning feature. It is really helpful.

[benjamingr]

It might be useful to be able to define a filter.

[petkaantonov]

@benjamingr you already can, e.g. return null. It is documented even http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-created-in-a-handler-but-none-were-returned-from-it

User.register(...);
return null;

[benjamingr]

@petkaantonov I mean filtering code areas where you don't want to get warnings from. This is typically called "Just my code".

[rishabhmhjn]

Does it really has to be a null?

This one is working

return new Promise(resolve => {
  // do my calculations and get result
  resolve(result);
  return null;
});

Is it not OK if I do something like this ↓

This one isn't

return new Promise(resolve => {
  // do my calculations and get result
  return resolve(result);
});

[benjamingr]

@rishabhmhjn the return value of the promise constructor is ignored. Bluebird is right about warning you here - your return has no effect.

[c0debreaker]

We're getting the same error too. Adding the return null didn't help at all. We are using ampersand's fetch which don't return a promise.

    // get meta data about the catalog
    metaData = new CatalogMetaData( {id: this.id} );
    this.promises.metaData = new Promise(function(resolve, reject){
      metaData.fetch({
        success: function(){
          resolve(metaData);
          return null;
        },
        error: function(err){
          reject(err);
          return null;
        }
      });
    });

[wanming]

Same error when using co:

'use strict';

const db = require('../model/');

const File = {
  *index() {
    const count = yield db.File.count();
    yield this.render('index', { data: 'Hello, world!', count });
  },
};

module.exports = File;

Nothing changed after I add some data to the table files. Any updates?

[petkaantonov]

Is co creating bluebird promises? In any case you can just swap co for Promise.coroutine if you're using bluebird anyway.

[magicdawn]

I Got the same warning, and it warns a lot. I step into co, found out that

co(function*(){
  yield a_bluebird_promise;
  ...
})

1. we yield a_bluebird_promise
2. next(a_bluebird_promise) -> a_bluebird_promise.then(onFulfilled, onRejected) https://github.com/tj/co/blob/4.6.0/index.js#L100
3. next() in the onFulfilled() fulfill handler create a Promise and did not return https://github.com/tj/co/blob/4.6.0/index.js#L85

So, it's co's problem~ Or we should modify the warning strategy? Not when a handle create a promise but not return it, but when a case like this is continued with a then call

e.g

not warn this

awesomePromise
.then(function(val){
  doSomeThing(val); // return a promise, but not returned
})

warn a next then call

awesomePromise
.then(function(val){
  doSomeThing(val); // return a promise, but not returned
})
.then(function(){
  // blabla
})

cause previous then call create a promise but not returned, warn on this then call

[magicdawn]

BTW, using co@4.6.0 bluebird@3.3.3

[petkaantonov]

why use co when bluebird already comes with Promise.coroutine

[magicdawn]

maybe co.wrap is shorter than Promise.coroutine :joy:

[petkaantonov]

var co = require("bluebird").coroutine :P

[petkaantonov]

Your suggestion is very interesting, I'll consider it

[benjamingr]

This is a problem since the chain is detached so I'm +1 on keeping the current strategy - but I'd love for a stronger way to opt-out of warnings (for example, we could check in the stack trace if the error originated in node_modules and if it did not present it).

[magicdawn]

Fixed in co, see tj/co#259, not published yet

[devotox]

I get this error when i do something like

promise.resolve(result).then(function(result){
            var _code = ( result && result.code ) || response_status.internal_success;
            return response.status(_code).json( ( result && result.response ) || result || []);
        }).catch(function(result){
            var _code = ( result && result.code ) || response_status.internal_error;
            return !response.headersSent ? response.status(_code).send(get_error_message(_code)) : get_error_message(_code);
        });

and even a return before the promise.resolve gives me nothing Stack trace

api_1             | [ Nodemon ][ Error ] Warning: a promise was created in a handler but was not returned from it
api_1             |     at Object.__default__ (/var/app/current/api/common/utils/api_response_handler.js:181:18)
api_1             |     at Object.exports.switch_case (/var/app/current/api/common/utils/utils.js:77:51)
api_1             |     at Seneca.<anonymous> (/var/app/current/api/common/utils/api_response_handler.js:215:17)
api_1             |     at act_done (/var/app/current/api/node_modules/seneca/seneca.js:1220:21)
api_1             |     at /var/app/current/api/node_modules/gate-executor/gate-executor.js:155:20
api_1             |     at /var/app/current/api/common/utils/api_response_handler.js:262:12
api_1             |     at processImmediate [as _immediateCallback] (timers.js:383:17)
api_1             | From previous event:
api_1             |     at Object.__default__ (/var/app/current/api/common/utils/api_response_handler.js:257:27)
api_1             |     at Object.exports.switch_case (/var/app/current/api/common/utils/utils.js:77:51)
api_1             |     at null.callback (/var/app/current/api/common/utils/api_response_handler.js:322:18)
api_1             |     at Query.handleReadyForQuery (/var/app/current/api/node_modules/pg/lib/query.js:89:10)
api_1             |     at null.<anonymous> (/var/app/current/api/node_modules/pg/lib/client.js:163:19)
api_1             |     at emitOne (events.js:95:20)
api_1             |     at emit (events.js:182:7)
api_1             |     at Socket.<anonymous> (/var/app/current/api/node_modules/pg/lib/connection.js:109:12)
api_1             |     at emitOne (events.js:90:13)
api_1             |     at Socket.emit (events.js:182:7)
api_1             |     at readableAddChunk (_stream_readable.js:153:18)
api_1             |     at Socket.Readable.push (_stream_readable.js:111:10)
api_1             |     at TCP.onread (net.js:529:20)

[spion]

FWIW I agree that the warning is a bit too opinionated and it seems like the suggestion proposed with @magicdawn should reduce false (or rather "opinionated") positives to zero

[mathieug]

I get the warning with:

userSchema.statics.createUser = function() {
  const User = this || mongoose.model('User');

  return User.create({
    // ...
  });
};

Update: It came from co, I've replaced it by bluebird.coroutine(). Thanks @petkaantonov.

[benjamingr]

Quoting Gorgi from FB:

if you aren't attaching another handler then you cannot possibly care to wait for the operation

That makes sense, I changed my mind and I'm 100% with the change of the warning strategy.

Cheers.

[petkaantonov]

@magicdawn I added your suggestion in 3.3.4

[magicdawn]

Nice :smile:

[nfantone]

I realize this a closed issue, but since it has had activity in the last few days, I'm going to comment on this.

I'm experiencing this same warning. Happens in the following snippet, inside a class function:

    authenticate() {
      return (args, done) => {
        return this.strategy.authenticate(args)
          .then((profile) => provider.jwt(this.seneca, profile))
          .then((token) => done(null, { // <-- This line triggers the warning
            ok: true,
            token: token
          }))
          .catch((err) => done(null, {
            ok: false,
            why: err
          }));
      };
    }

And I don't see who's the culprit here. done is just a callback. And even if it weren't, I am returning it. What's going on here, @petkaantonov?

[petkaantonov]

calling done is calling some other function that return a promise

[petkaantonov]

Basically something like this is happening:

function somethingElse() {
    return new Promise(...)
}

function something() {
    somethingElse()
}

.then(function() {
    something();
})

You can just use asCallback:

authenticate() {
      return (args, done) => {
        return this.strategy.authenticate(args)
          .then((profile) => provider.jwt(this.seneca, profile))
          .then((token) => {
            return {ok: true, token: token};
          })
          .catch((e) => {
            return {ok: false, why: e};
          })
         .asCallback(done);
      };
    }

[nfantone]

Thanks for your reply, @petkaantonov.

A few pointers:

In your first snippet, I think this

.then(function() {
    something();
})

Should really be return something();, to match my scenario.

But, that aside, if this is what's actually happening, I tend to think that the warning is not properly in place since done is a callback from some library that I do not control. Nor should I really care about it. Refactoring my own code base to get rid of a warning of yet another library sounds fishy, even if it is truthful.

Any thoughts?