Multiple promises in Node js

Question

I am new to nodejs and struggling to find the issue with the below piece of code.

All i am trying to achieve is, calling few methods one after other and if all the calls are successful, then return success else throw an error if any of the method fails.

Issue i have is, before all the method gets executed, the main method terminates even though the promises are yet to be fullfilled.

Main method

processRequest(neworder).then(function (done) {
    console.log('Successfully processed the order' + done);
    res.sendStatus(200);
}).fail(function (error) {
    res.status(404).send(error);
})

Other method calls

module.exports.processRequest = function (Order) {
    var deferred = Q.defer();

    findX(Order)
        .then(findYBasedOnPrevOutput)
        .then(findZBasedOnPrevOutput)
        .then(deferred.resolve())
        .fail(function (err) {
            console.log('Failed to process request' + err);
            deferred.reject(err);
        });

    return deferred.promise;
}


var findX = function (order) {
    var deferred = Q.defer()

    db.list(order, function (address) {
        console.log('Query success');
        if (address == null)
            deferred.reject('Error');
        else {
            deferred.resolve(address);
        }
    })

    return deferred.promise;
};

Issue that i am having is, i see in the console the success just after the findX method gets called. I expected the success msg after findZ method.

Can you please help me in finding the issue with the above code, appreciate your input/suggestions in this regard

For simplicity, i didn't share the other modules here, but they are very similar to findX

` .then(deferred.resolve())` should be ` .then(() => deferred.resolve())` - what you're doing is like setTimeout(fn(), 100) instead of setTimeout(fn, 100). Also, you're doing explicit construction: stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it — Benjamin Gruenbaum
– Benjamin Gruenbaum, Commented Nov 19, 2015 at 12:21
@BenjaminGruenbaum.. Thanks it helped. Changing the .then(deferred.resolve()) to .then(deferred.resolve) fixed the issue...And thanks for sharing the wonderful link. — Raj
– Raj, Commented Nov 19, 2015 at 19:37
@BenjaminGruenbaum Can you post your response as answer, so that i can accept. — Raj
– Raj, Commented Nov 22, 2015 at 10:16
No, you should probably delete your quesiton as it's a typo issue. — Benjamin Gruenbaum
– Benjamin Gruenbaum, Commented Nov 22, 2015 at 11:55

jfriend00 · Accepted Answer · 2015-11-19 22:29:25Z

1

I'd suggest you just use the promise you already have rather than creating a new one as both more efficient and to avoid a promise anti-pattern:

module.exports.processRequest = function (Order) {
  return findX(Order)
    .then(findYBasedOnPrevOutput)
    .then(findZBasedOnPrevOutput)
    .fail(function (err) {
      // log the error, then throw it again so it is returned as a rejected promise
      console.log('Failed to process request' + err);
      throw(err);
    });
}

And, I'd suggest changing findX to use Q's ability to turn a standard async call into one that returns a promise like this:

var findX = function(order) {
  return Q.nfcall(db.list.bind(db), order);
};

Or, combine the two like this:

module.exports.processRequest = function (Order) {
  return Q.nfcall(db.list.bind(db), Order)
    .then(findYBasedOnPrevOutput)
    .then(findZBasedOnPrevOutput)
    .fail(function (err) {
      // log the error, the throw it again so it is returned as a rejected promise
      console.log('Failed to process request' + err);
      throw(err);
    });
}

edited Nov 19, 2015 at 22:29

answered Nov 19, 2015 at 22:22

jfriend00

711k104 gold badges1.1k silver badges1k bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Raj Over a year ago

Thanks for the response, was googling to understand more on the nfcall, looks to me, its a handy method to wrap around all the legacy callback methods. But given all my methods are new, wondering will that be worth spending another day to get them changed to return the promise as explained in the 1st response, or should i just wrap them with nfcall. Which is the best way, any suggestion over there.. Again thanks for taking time to respond..

jfriend00 Over a year ago

@Shiv - Once you get comfortable with promises, you will find that you pretty much want to use them for ALL your async operations as they offer significant advantages in coordinating multiple async operations and in error handling and in code readability. So, I'd suggest you "promisify" all the async functions you use. I use Bluebird's .promisifyAll() which will promisify the entire interface from a module in one line of code.

jfriend00 Over a year ago

@Shiv - It's generally best to promisify things once at the beginning of a module so that overhead is done and you can then just program with promises, but if there's a one-off async function, you can certainly use Q.nfcall() with it as needed.

Collectives™ on Stack Overflow

Multiple promises in Node js

1 Answer 1

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related