2

I have an async function processWaitingList() that needs to wait for the result of another function findOpenVillage() that gets it's data from my database.

I'm getting the following error and do not understand why:

(node:2620) UnhandledPromiseRejectionWarning: TypeError: Cannot read property '0' of undefined at processWaitingList (xx\server.js:151:36) at process._tickCallback (internal/process/next_tick.js:68:7) (node:2620) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)

I've read everything I can find on promises, callbacks, async and await, but I just cannot figure out how these work.

function slowServerLoop() {
    processWaitingList();
}
setInterval(slowServerLoop, 500);

async function processWaitingList() {
    let openVillage = await findOpenVillage();
    villageId = openVillage[0];
    openSpots = openVillage[1];
    addPlayerToVillage(waitingList[0], villageId, openSpots);
}

function findOpenVillage() {
    con.query(`SELECT id, maxVillagers FROM villages WHERE status='0'`, function (err, result, fields) {
        if (err) throw err;
        if (result.length === 1) {
            let villageId = result[0].id;
            let maxVillagers = result[0].maxVillagers;
            con.query(`SELECT COUNT(*) as villagerCount FROM villagers WHERE villageId='${villageId}'`, function (err, result, fields) {
                if (err) throw err;
                let villagerCount = result[0].villagerCount;
                let openSpots = maxVillagers - villagerCount;
                return [villageId, openSpots];
            });
        }
    });
}
Pascal Claes
  • 161
  • 6
  • Possible duplicate of [How do I return the response from an asynchronous call?](https://stackoverflow.com/questions/14220321/how-do-i-return-the-response-from-an-asynchronous-call) – nicholaswmin Feb 08 '19 at 14:14
  • I was just reading that one, trying to use the suggested option: ES2017+: Promises with async/await - but can't figure out how to get it to work – Pascal Claes Feb 08 '19 at 14:16
  • 1
    You need to convert `con.query` into a Promise so you can `await` it. – nicholaswmin Feb 08 '19 at 14:17

3 Answers3

2

there is important thing about await. If you put it behind some promise, it will await the promise to be resolved and then return the value to your code.

In your case, you are not returning promise, so there is nothing to be awaited.

When you need to combine promises with callbacks (async/await is just extension over promise chains), one of way is to wrap callback inside new promise.

In your case

return new Promise((resolve, reject) =>
{
con.query(`SELECT id, maxVillagers FROM villages WHERE status='0'`, function (err, result, fields) {
        if (err) { return reject(err); }
        if (result.length === 1) {
            let villageId = result[0].id;
            let maxVillagers = result[0].maxVillagers;
            con.query(`SELECT COUNT(*) as villagerCount FROM villagers WHERE villageId='${villageId}'`, function (err, result, fields) {
                if (err) { return reject(err); }
                let villagerCount = result[0].villagerCount;
                let openSpots = maxVillagers - villagerCount;
                resolve([villageId, openSpots]);
            });
        }
    });
}
   

Also remember that you need to return it (its already in code ^^ )

libik
  • 22,239
  • 9
  • 44
  • 87
  • @ExplosionPills the function is not vulnerable to injection because it does not take any user input – Pascal Claes Feb 08 '19 at 14:26
  • @libik Thanks a lot! I could've never figured this out myself! This seems to be working! – Pascal Claes Feb 08 '19 at 14:27
  • Imagine a bad actor who is able to create a `villageId` with an injection – Explosion Pills Feb 08 '19 at 14:27
  • 1
    @ExplosionPills - I just took the OP code and wrap it with Promise, which I also described in my answer... So I dont think it make sense to evaluate the code quality itself. – libik Feb 08 '19 at 14:27
  • This is not a question of quality, it's a question of security. I think it's reasonable to propagate good security practices in SO answers. – Explosion Pills Feb 08 '19 at 14:31
  • @ExplosionPills IMO that's not a reason for downvote. Perhaps just leave it as a comment next time so as not to punish the answerer who doesn't get paid to answer the question in the 1st place. SO answers aren't supposed to fix secondary code issues, +1. Also I still don't see how this is vulnerable to SQL injection since it doesn't take user input but that's besides the point. – nicholaswmin Feb 08 '19 at 14:49
2

findOpenVillage does not return a Promise. You need it to return a Promise in order to use async/await.

If possible, you should see if con.query has the ability to return a Promise. There is the mysql-promise library that you could use, among others. You could also use the Promise constructor yourself or the built in util.promisify. Let's use that last option as an example:

const util = require('util');

...
async function findOpenVillage() {
  const query = util.promisify(con.query);
  const maxResult = await query(`SELECT id, maxVillagers FROM villages WHERE status = '0'`);
  if (maxResult.length === 1) {
    const villageId = result[0].id;
    const maxVillagers = result[0].maxVillagers;
    const countResult = await query(
      `SELECT COUNT(*) as villagerCount FROM villagers WHERE villageId = ?`,
      villageId
    );
    const villagerCount = result[0].villagerCount;
    const openSpots = maxVillagers - villagerCount;
    return [villageId, openSpots];
  }
}

I've made this an async function so you could use await, but using promises with .then would also be reasonable. If you did that, you would have to return the promise.


Your original code was using string interpolation instead of parameterized queries. This makes the query vulnerable to injection. I have corrected this in my answer as well.

Explosion Pills
  • 188,624
  • 52
  • 326
  • 405
  • Thanks for adding the parameterized queries, I'll definetely use those! – Pascal Claes Feb 08 '19 at 14:29
  • What is const util = require('util'); - what makes this method better than returning a Promise? – Pascal Claes Feb 08 '19 at 14:39
  • @PascalClaes https://nodejs.org/api/util.html#util_util_promisify_original — it’s hard to say if an approach is better than another. You’re already using an async function, so consistentcy is nice. Async functions implicitly return Promises. – Explosion Pills Feb 08 '19 at 15:16
-1

You need to access the code after the promise is fullfilled. Your function findOpenVillage() must return a promise object. You need to access the values in then. It means that you will be accessing the values after the promise is fullfilled.

openVillage.then(function(response){
villageId = openVillage[0];
    openSpots = openVillage[1];
    addPlayerToVillage(waitingList[0], villageId, openSpots);
});
Shahzeb Khan
  • 3,582
  • 8
  • 45
  • 79
  • Hi, sorry you are not correct about `then`, it works same as the `await`. You are correct about returning promise though. – libik Feb 08 '19 at 14:18