1

Why a different result in the following cases? The first example works correctly, returns an array of three elements ["qwe", "rty", "asd"]. Second example return only last element ["asd"]. Please, explain how it work? Why is this behavior happening?

In the first example working through intermediate variable awaitResult.

class XXX {
  constructor() {
    this.storage = {1: ['qwe'], 2: ['rty'], 3: ['asd']}
  }

  async getValue(key) {
    return this.storage[key];
  }

  async logValues() {
    let keys = [1, 2, 3]
    let values = []

    // ----- First version -----

    await Promise.all(
      keys.map(
        async key => {
          let awaitResult = await this.getValue(key)
          values = values.concat(awaitResult)
        }
      )
    );

    console.log(values)
  }
}

let xxx = new XXX()
xxx.logValues()

In the second example working without awaitResult.

class XXX {
  constructor() {
    this.storage = {1: ['qwe'], 2: ['rty'], 3: ['asd']}
  }

  async getValue(key) {
    return this.storage[key];
  }

  async logValues() {
    let keys = [1, 2, 3]
    let values = []

    // ----- Second version -----
   
    await Promise.all(
      keys.map(
        async key => values = values.concat(await this.getValue(key)),
      )
    );

    console.log(values)
  }
}

let xxx = new XXX()
xxx.logValues()

7
  • This is an odd way to build your values array. Why not use values = await Promise.all(...), with the promise resolution function returning the actual data you need? Commented Oct 29, 2019 at 18:07
  • Your first version doesn't return any values from the map callback. I'm surprised it works. Commented Oct 29, 2019 at 18:07
  • 1
    @IceMetalPunk it works through two side-effects that are not design decisions. 1. .map shouldn't be used to do iteration and mutation. It's basically a forEach (or for) that is there. The mapping is irrelevant. 2. await Promise.all works but for a completely different reason. await will cause the async function to pause until the promise is resolved. await Promise.all returns a promise but since there is (essentially) no arguments it's given it just resolves automatically. This still is still queued on the event loop after all the awaits in the map callbacks are resolved. Commented Oct 29, 2019 at 18:19
  • @IceMetalPunk here is a JSBin of this re-written slightly more clearly showing what happens. The await Promise.all is a bit misleading, so I've moved that as a separate statement await new Promise((r) => {r()}); (await an auto-resolved promise) to show what role it has in the process. Commented Oct 29, 2019 at 18:24
  • 1
    @IceMetalPunk it's absolutely bad practive, you're not wrong. .map shouldn't be used as it's used here for iteration and await Promise.all shouldn't be used if you're not awaiting promises. Using an external shared values is also a bad idea. All in all, this should be done through the promises API - do use Promise.all but give it multiple promises and use .then() to combine the results that come back and then return them. Commented Oct 29, 2019 at 18:31

3 Answers 3

6

The answer from Jonas Wilms is absolutely correct. I just want to expand upon it with some clarification, as there are two key things one needs to understand:

Async functions are actually partially synchronous

This, I think, is the most important thing. Here is the thing - knowledge of async functions 101:

  1. They will execute later.
  2. They return a Promise.

But point one is actually wrong. Async functions will run synchronously until they encounter an await keyword followed by a Promise, and then pause, wait until the Promise is resolved and continue:

function getValue() {
  return 42;
}

async function notReallyAsync() {
  console.log("-- function start --");
  
  const result = getValue();
  
  console.log("-- function end --");
  
  return result;
}


console.log("- script start -");

notReallyAsync()
  .then(res => console.log(res));

console.log("- script end -");

So, notReallyAsync will run to completion when called, since there is no await in it. It still returns a Promise which will only be put on the event queue and resolved on a next iteration of the event loop.

However, if it does have await, then the function pauses at that point and any code after the await will only be run after the Promise is resolved:

function getAsyncValue() {
  return new Promise(resolve => resolve(42));
}

async function moreAsync() {
  console.log("-- function start --");
  
  const result = await getAsyncValue();
  
  console.log("-- function end --");
  
  return result;
}

console.log("- script start -");

moreAsync()
  .then(res => console.log(res));

console.log("- script end -");

So, this is absolutely the key to understanding what's happening. The second part is really just a consequence of this first part

Promises are always resolved after the current code has run

Yes, I mentioned it before but still - promise resolution happens as part of the event loop execution. There are probably better resources online but I wrote a simple (I hope) outline of how it works as part of my answer here. If you get the basic idea of the event loop there - good, that's all you need, the basics.

Essentially, any code that runs now is within the current execution of the event loop. Any promise will be resolved the next iteration the earliest. If there are multiple Promises, then you might need to wait few iterations. Whatever the case, it happens later.

So, how this all applies here

To make it more clear, here is the explanation: Code before await will be completed synchronously with the current values of anything it references while code after await will happen the next event loop:

let awaitResult = await this.getValue(key)
values = values.concat(awaitResult) 

means that the value will be awaited first, then upon resolution values will be fetched and awaitResult will be concatenated to it. If we represent what happens in sequence, you would get something like:

let values = [];

//function 1: 
let key1 = 1;
let awaitResult1;
awaitResult1 = await this.getValue(key1); //pause function 1 wait until it's resolved

//function 2:
key2 = 2;
let awaitResult2;
awaitResult2 = await this.getValue(key2); //pause function 2 and wait until it's resolved

//function 3:
key3 = 3;
let awaitResult3;
awaitResult3 = await this.getValue(key3); //pause function 3 and wait until it's resolved

//...event loop completes...
//...next event loop starts 
//the Promise in function 1 is resolved, so the function is unpaused
awaitResult1 = ['qwe'];
values = values.concat(awaitResult1);

//...event loop completes...
//...next event loop starts 
//the Promise in function 2 is resolved, so the function is unpaused
awaitResult2 = ['rty'];
values = values.concat(awaitResult2);

//...event loop completes...
//...next event loop starts 
//the Promise in function 3 is resolved, so the function is unpaused
awaitResult3 = ['asd'];
values = values.concat(awaitResult3);

So, you would get all of the values added correctly together in one array.

However, the following:

values = values.concat(await this.getValue(key))

means that first values will be fetched and then the function pauses to await the resolution of this.getValue(key). Since values will always be fetched before any modifications have been made to it, then the value is always an empty array (the starting value), so this is equivalent to the following code:

let values = [];

//function 1:
values = [].concat(await this.getValue(1)); //pause function 1 and wait until it's resolved
//       ^^ what `values` is always equal during this loop

//function 2:
values = [].concat(await this.getValue(2)); //pause function 2 and wait until it's resolved
//       ^^ what `values` is always equal to at this point in time

//function 3:
values = [].concat(await this.getValue(3)); //pause function 3 and wait until it's resolved
//       ^^ what `values` is always equal to at this point in time

//...event loop completes...
//...next event loop starts 
//the Promise in function 1 is resolved, so the function is unpaused
values = [].concat(['qwe']);

//...event loop completes...
//...next event loop starts 
//the Promise in function 2 is resolved, so the function is unpaused
values = [].concat(['rty']);

//...event loop completes...
//...next event loop starts 
//the Promise in function 3 is resolved, so the function is unpaused
values = [].concat(['asd']);

Bottom line - the position of await does affect how the code runs and can thus its semantics.

Better way to write it

This was a pretty lengthy explanation but the actual root of the problem is that this code is not written correctly:

  1. Running .map for a simple looping operation is bad practice. It should be used to do a mapping operation - a 1:1 transformation of each element of the array to another array. Here, .map is merely a loop.
  2. await Promise.all should be used when there are multiple Promises to await.
  3. values is a shared variable between async operations which can run into common problems with all asynchronous code that accesses a common resource - "dirty" reads or writes can change the resource from a different state than it actually is in. This is what happens in the second version of the code where each write uses the initial values instead of what it currently holds.

Using these appropriately we get:

  1. Use .map to make an array of Promises.
  2. Use await Promise.all to wait until all of the above are resolved.
  3. Combine the results into values synchronously when the Promises have been resolved.

class XXX {
  constructor() {
    this.storage = {1: ['qwe'], 2: ['rty'], 3: ['asd']}
  }

  async getValue(key) {
  console.log()
    return this.storage[key];
  }

  async logValues() {
  console.log("start")
    let keys = [1, 2, 3]

    let results = await Promise.all( //2. await all promises
      keys.map(key => this.getValue(key)) //1. convert to promises
    );
    
    let values = results.reduce((acc, result) => acc.concat(result), []); //3. reduce and concat the results
    console.log(values);
  }
}

let xxx = new XXX()
xxx.logValues()

This can also be folded into the Promise API as running Promise.all().then:

class XXX {
  constructor() {
    this.storage = {1: ['qwe'], 2: ['rty'], 3: ['asd']}
  }

  async getValue(key) {
  console.log()
    return this.storage[key];
  }

  async logValues() {
  console.log("start")
    let keys = [1, 2, 3]

    let values = await Promise.all( //2. await all promises
      keys.map(key => this.getValue(key)) //1. convert to promises
    )
    .then(results => results.reduce((acc, result) => acc.concat(result), []));//3. reduce and concat the results
     
    console.log(values);
  }
}

let xxx = new XXX()
xxx.logValues()

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

Comments

2

Concurrency. Or more precisely: A non atomic modification of values.

First of all, the values.concat(...) get evaluated, at that time values is an empty array. Then all the functions await. Then, all the values = get run, concatenating the awaited element to the empty array, and assigning those arrays with one value to values. The last resolved value wins.

To fix:

 await Promise.all(
  keys.map(
    async key => {
       const el = await this.getValue(key); // async operation
      values = values.concat(el); // atomic update
    }
  )
);

3 Comments

OK, this makes sense now. I was wondering what's happening but you're right. Just to clarify if somebody else still doesn't get it - const el = await something() followed by value.concat(el) will resolve values after the async operation finishes. while values.concat(await something()) will first resolve values as [] and then pause for the async operation to finish. So the equivalent code in the second case is [].concat(await something()) as values will always be an empty array before firing the async operations.
@VLAZ I'm quite busy right now, feel free to clarify the answer :)
@JonasWilms Thanks for the quick and short answer.
0

You want to change how you're computing values, because you can make Promise.all entirely responsible for this:

  async logValues() {
    const mapFn = async(key) => this.getValue(key);
    const values = await Promise.all(this.keys.map(mapFn));
    console.log(values)
    return values;
  }

Note that this works because we're using a one-line arrow function: it automatically returns the result of the function statement (which is not the case when you split your arrow function body over multiple lines with curly brackets).

Also I assume keys isn't actually the array [1,2,3], because that would be weird, but if you do need a sequence of numbers, and you don't want to hardcode that array, new Array(n).fill().map( (_,index) => console.log(index) ) where n is some number should do the trick.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.