Issue resolving all promises

Question

I have a code block where I want to concatenate results of two database queries. So I tried implementing Promises.all

 const promise_list = []
let appData = [];
let promise = new Promise((resolve, reject) => {
    let database = new Database();
    database.query(`select * from configuration where  version = (select max(version) from configuration) OR version= ? ORDER BY version `, [req.body.app_version])
        .then(rows => {
            appData=rows[0];
            database.close()
            resolve()
        }, err => {
            return database.close().then(() => { throw err; })
        })
        .catch(err => {
            console.log(err);
            res.status(500).json("Database Error");
            reject()
        })
});
promise_list.push(promise)
let promise2 = new Promise((resolve, reject) => {
    let database = new Database();
    database.query(`select points from users where id=?`, [req.user.id])
        .then(rows => {
            appData.points=rows[0]['points'];
            database.close()
            resolve()
        }, err => {
            return database.close().then(() => { throw err; })
        })
        .catch(err => {
            console.log(err);
            res.status(500).json("Database Error");
            reject()
        })
});
promise_list.push(promise2)
Promise.all(promise_list).then(result => {
    res.status(200).json(appData);
});

The second query works sometimes and sometimes it doesnt. What could be the issue?

what do you mean _The second query works sometimes and sometimes it doesnt_ ? Does it throw an error? Whats the expected and actual behavior? — Ayush Gupta, Nov 19 '18 at 07:31
*"The second query works sometimes and sometimes it doesnt."* What exactly does that mean? What happens when it "doesn't work"? — Felix Kling, Nov 19 '18 at 07:32
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! And don't do `res.status(500)` twice - do it once in the end. — Bergi, Nov 19 '18 at 07:32
Call database.close in your promise.all callback not each query call — dotconnor, Nov 19 '18 at 07:33
@AyushGupta second query appends data to appData variable, sometimes its not there — Blaze Mathew, Nov 19 '18 at 07:36
@FelixKling second query appends data to appData variable, sometimes its not there — Blaze Mathew, Nov 19 '18 at 07:37
`appData.points=rows[0]['points'];` only works if `appData` has been initialized by the other promise. But with `Promise.all`, either promise can be resolved first. If you need them to resolve in order, then you should do `promise1.then(() => /*promise2 stuff */).then(...)` instead. If they can run in parallel, just have each of them resolve properly, don't try to make them share an object. — Felix Kling, Nov 19 '18 at 07:39
@FelixKling got it. so in cases where I dont get points is because second promise resolved first right. Thanks — Blaze Mathew, Nov 19 '18 at 07:42

Felix Kling · Accepted Answer · 2018-11-19T07:53:33.427

appData.points=rows[0]['points']; only works if appData has been initialized by the other promise first. But with Promise.all, either promise can be resolved first. If the first promise resolve second, it will simply override whatever value appData currently has.

It looks like you are using promises incorrectly. Instead of using them with side-effects (assigning to appData), you should resolve them properly.

The whole code can be cleaned up a lot to something like this:

let database = new Database();
let promise = database.query(`select * from configuration where  version = (select max(version) from configuration) OR version= ? ORDER BY version `, [req.body.app_version])
  .then(rows => rows[0]);
let promise2 = database.query(`select points from users where id=?`, [req.user.id])
  .then(rows => rows[0].points);

Promise.all([promise, promise2])
  .then(
    ([appData, points]) => {
      appData.points = points;
      res.status(200).json(appData);
    },
    err => {
      console.log(err);
      database.close().then(() => {
        res.status(500).json("Database Error");
      });
    }
  );

Don't know what Database does, so it's not clear whether it's OK to only call it once. But it should you a better idea for how to use promises.

Issue resolving all promises

1 Answers1