CODEWITHSUNDEEP
node.jspromisebluebirdasync.js
swateek
swateek

Reputation: 7570

Bluebird Warning : a promise was created in a handler but was not returned from it

I have an array of filenames which I iterate using this async module for node.

       async.eachSeries(imageStore, function(imageDetails,callback){
            mongoMan.updateCollection('imageStore',imageDetails,{_id: imageDetails.fileName}).then(function(res){
                  return callback(null, res);
                }).catch(function(err){
                  logger.error(err);
                  return callback(err);
                });
            },function(err){
              callback(null);
            });

The updateCollection() function is like this:

exports.updateCollection = function(collection, values, condArr){

  var result = imageStores.updateAsync(condArr, values, { upsert: true }).then(function(res){
    return new Promise.resolve(res);
  }).catch(function(err){
    logger.error(err);
  });

  return new Promise.resolve(result);
}

This code works well, updates DB and everything. But I am still unable to resolve the warning that bluebird throws:

Warning: a promise was created in a handler but was not returned from it
at Object.exports.updateCollection (/home/swateek/Documents/codebase/poc/apps/webapp/server/components/mongodb/mongoConn.js:46:22)
at /home/swateek/Documents/codebase/poc/apps/webapp/server/components/imageStore.js:72:24
at /home/swateek/Documents/codebase/poc/apps/webapp/node_modules/async/lib/async.js:181:20
at iterate (/home/swateek/Documents/codebase/poc/apps/webapp/node_modules/async/lib/async.js:262:13)

Have looked up solutionhere but that ain't convincing enough in my case. Atleast is there a way to turn off the warning?

UPDATE

Please check the correct answer below, and here's how my calling function looks like:

function(callback){// post imageStore data to DB
          async.eachSeries(imageStore, function(imageDetails,callback){
            mongoMan.updateCollection('imageStore',imageDetails,{_id: imageDetails.fileName}).catch(function(err){
                  logger.error(err);
                  return callback(err);
                });
            return callback(null);
            },function(err){
              callback(null);
            });
        }

Upvotes: 1

Views: 1622

Answers (2)

jfriend00
jfriend00

Reputation: 707366

You are asking for trouble and throwing away programming advantages if you try to mix async library callbacks with promises. Pick one structure or the other and use it everywhere. Personally, I'd suggest you move to promises and just "promisify" functions that currently work with callbacks. If you're using the Bluebird Promise library, then it has Promise.promisify() and Promise.promisifyAll() which make it pretty easy to promisify things that use the standard node.js async callback so you can just control everything with Promise logic.

Now, onto your specific issue. That error means that you are creating promises within a .then() handler, but those promises are not returned or chained onto any previous promise. Thus, they are completely independent from your other chain. This is usually a bug (thus the reason for the warning). The only time one might actually want to do that is you have some fire and forget operation that you don't want the outer promise to wait for and you aren't tracking errors on which is not what you have here.

Change to this:

exports.updateCollection = function(collection, values, condArr){
  return imageStores.updateAsync(condArr, values, { upsert: true}).catch(function(err){
    logger.error(err);
    // rethrow error so the caller will see the rejection
    throw err;
  });
}

Changes:

  1. Return the main promise rather than creating a new promise.
  2. No need for return new Promise.resolve(res); as res is already returned from that promise so you can remove that whole .then() handler as it isn't doing anything.
  3. No need for return new Promise.resolve(result); as you can just return the earlier promise directly.

FYI, though you don't need it here at all, Promise.resolve() can be called directly without new.

Upvotes: 4

Paul
Paul

Reputation: 141827

You have some issues here:

 var result = imageStores.updateAsync(condArr, values, { upsert: true }).then(function(res){
    return new Promise.resolve(res);
 }).catch(function(err){
    logger.error(err);
 });

 return new Promise.resolve(result);

Firstly: Promise.resolve is not a constructor so it shouldn't be used with new. Secondly: there is no point calling Promise.resolve( result ) at all, just return result which is already a Promise. The intermediate then is pointless as well. You can reduce that code to:

return imageStores.updateAsync(condArr, values, { upsert: true })
  .catch( function(err){
      logger.error(err);
  } )
;

Upvotes: 0

Related Questions