JS promise losing data variable

Question

I faced an issue with this promise. After forEach with function that returns object of users, the variable of arrays (clients) becomes empty ( [] ). Ngl I don't have a single clue how to solve this problem. Thanks for your time :))

function getAdminUsers(adminGroups) {
        return new Promise(async (resolve) => {
          try {
            var clients = new Array();
            console.log(adminGroups);
            await adminGroups.forEach(async (group) => {
              clients[group] = await teamspeak.serverGroupClientList(group);
            });
            console.log('CLIENTS', clients);
            resolve(clients);
          } catch (error) {
            handleServerQueryError(error);
          }
        });
      }

adminGroups array:

module.exports = {
  adminGroups: [6, 277, 14, 18, 17, 16, 13, 528, 568],
};

the terminal output:

New user connected from 188.146.229.188
[
    6, 277, 14,  18,
   17,  16, 13, 528,
  568
]
CLIENTS []

This construct `await adminGroups.forEach(...)` doesn't work. Either use a regular loop with `await` or check out [Promise#all()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all). — Thomas, Jan 25 '21 at 00:44
And please don't do this `new Promise(async ...)` every `async function` will **always** return a Promise. So what do you need `new Promise()` for? Plus, in your particular case, the part with `catch (error)` will create a memory leak as it doesn't ever settle the `new Promise()` you've created. — Thomas, Jan 25 '21 at 00:50
@Thomas Never-resolved promises don't leak memory, they're garbage-collected like everything else. — Bergi, Jan 25 '21 at 01:29

Chayim Friedman · Answer 1 · 2021-01-25T01:23:42.133

forEach() isn't async. It returns undefined, which, because you await it, converts automatically to Promise that immediately resolves to undefined. The callbacks are run, but you don't wait for them to finish.

There are two different things you can do to await some Promises:

If you want for them to run one-after-one (like forEach()), you can use a for-of loop. Also note that I removed the return new Promise(async resolve => ...), because an async function always return a promise, so this is redundant.

async function getAdminUsers(adminGroups) {
  try {
    var clients = new Array();
    console.log(adminGroups);
    for (const group of adminGroups) {
      clients[group] = await teamspeak.serverGroupClientList(group);
    }
    console.log('CLIENTS', clients);
    return clients;
  } catch (error) {
    handleServerQueryError(error);
  }
}

The other thing you can do is to use Promise.all() (or variants, like Promise.allSettled()) to run the promises in parallel. It can look like:

async function getAdminUsers(adminGroups) {
  try {
    var clients = new Array();
    console.log(adminGroups);
    await Promise.all(adminGroups.map(async group => {
      clients[group] = await teamspeak.serverGroupClientList(group);
    }));
    console.log('CLIENTS', clients);
    return clients;
  } catch (error) {
    handleServerQueryError(error);
  }
}

Using `.map` with a separate `clients` array in your second example is redundant, at that point it could just be `clients = await Promise.all...` — Zac Anger, Jan 25 '21 at 00:52
Not so, because in the original code not all indices of `clients` are assigned (there can be holes), and I wanted to preserve that — Chayim Friedman, Jan 25 '21 at 00:53
Ah, I assumed that was also a bug and that what OP wanted was something more like `.forEach((group, i) => { clients[i] = ...` — Zac Anger, Jan 25 '21 at 00:54

score 1 · Accepted Answer · answered Jan 25 '21 at 00:48

1

There's no need for the Promise constructor in your code, and you seem to be assigning items into an array using what could be property names rather than indexes (forEach((group, idx) and using idx is probably what you would want, or clients.push). There's no need create a new Array and push to it though, when you can use Promise.all on a .map.

async function getAdminUsers(adminGroups) {
  try {
    const clients = await Promise.all(
      adminGroups.map(async (group) => {
        const list = await teamspeak.serverGroupClientList(group)
        return list
      })
    )
    return clients
  } catch (error) {
    handleServerQueryError(error)
  }
}

answered Jan 25 '21 at 00:48

Zac Anger

6,983
2
15
42

2

What about `adminGroups.map(group => teamspeak.serverGroupClientList(group))` would this work? – Thomas Jan 25 '21 at 00:54
Yes @Thomas - good point. that would work for sure. – Randy Casburn Jan 25 '21 at 00:55
@Thomas That would work, there are people that prefer either style of writing – Chayim Friedman Jan 25 '21 at 00:57
Yeah, since it's returning a promise either way and and it's wrapped in the Promise.all, that should work just the same. – Zac Anger Jan 25 '21 at 01:01

JS promise losing data variable

2 Answers2