setTimeout does not produce correct async behavior

Question

I had the following question on an interview. cannot see what's wrong with it.

The following code attempts to run 10 tasks sequentially and have a timeout of 1 second after each task. Given the following piece of code, please identify issues with it, and write a correct implementation for the original intention.

for (let i = 0; i < 10; i++) {
  setTimeout(function() {
    // Running some asynchronous tasks here
    // ...
    console.log("completed task id " + i);
  }, 1000)
}

Does this answer your question? [JavaScript closure inside loops – simple practical example](https://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example) — VLAZ, Jan 16 '20 at 17:18
Oops, sorry wrong dupe...maybe? I've retracted the close vote but leave link just in case it's useful. — VLAZ, Jan 16 '20 at 17:19
Did the interviewers explain exactly what's meant by "*some asynchronous tasks here*" is this calling an async function, or is it running sequential code that is made async through the use of setTimeout? The two would have very different behaviour. — VLAZ, Jan 16 '20 at 17:20
timeouts all run at same time..... common question on here. "Why do all my messages appear at once?" — epascarello, Jan 16 '20 at 17:21
This runs all the tasks 1 second after you start, not with 1 second between them. — Barmar, Jan 16 '20 at 17:21
The problem is that every `console.log` will be written at the same time after 1 second. — Roberto Zvjerković, Jan 16 '20 at 17:21
@ritaj and also the `i` for all the timeouts will be the same, because the setTimeout's callback are all defined in the scope of the same loop. an IIFE could be used to pass each unique `i` to each callback, thus fixing the problem. this is a well known problem in js, and as thus this question should be closed as a duplicate. — r3wt, Jan 16 '20 at 17:22
@Barmar yet that still fits the description. Depending on how they interpret it - it runs some async tasks (the code delayed by setTimeout) with a timeout of 1 second after each task (the "task" is the timer and each will run after one second). So, this description *could be correct*. — VLAZ, Jan 16 '20 at 17:22
@VLAZ I think it's a stretch to interpret "task" as starting the timers rather than the functions run by the timer. And since the question implies that there's something wrong, we should interpret the language in the obvious way that makes that true. — Barmar, Jan 16 '20 at 17:25
@VLAZ well how bout that, i even learned something today "-) — r3wt, Jan 16 '20 at 17:26
@Barmar it's not like (bad) interviews try to trip you up with their wording, right... — VLAZ, Jan 16 '20 at 17:26

Barmar · Answer 1 · 2020-01-16T17:38:01.783

2

This starts all the timeouts immediately when the loop runs, so all the messages will be in 1 second, instead of 1 second between them.

One way to fix it is to make the timeouts depend on i.

for (let i = 0; i < 10; i++) {
  setTimeout(function() {
    // Running some asynchronous tasks here
    // ...
    console.log("completed task id " + i);
  }, 1000 * (i + 1))
}

This is the simplest way to do it, it starts each task 1 second after the previous one started.

But if the tasks performed some long operation, and you didn't want to start the next task until 1 second after the previous one completed, you would need a more complex implementation. Instead of using a loop, you would have to have the task increment i and call setTimeout() itself to schedule the next iteration.

function timeout_fn(i) {
  // do slow operation
  console.log("completed task id " + i);
  i++;
  if (i < 10) {
    setTimeout(() => timeout_fn(i), 1000);
  }
}

setTimeout(() => timeout_fn(0), 1000);

edited Jan 16 '20 at 17:38

answered Jan 16 '20 at 17:22

Barmar

741,623
53
500
612

I should note - if "some asynchronous tasks here" is supposed to be calling an async function, then you'd get the message *before* the async function is completed. Again, I really don't know what the interviewers' aim was here but I can find at least several different readings on the question and they all have different results. – VLAZ Jan 16 '20 at 17:25
@VLAZ Not if you `await` the async task. – Roberto Zvjerković Jan 16 '20 at 17:25
@VLAZ I already commented on that interpretation above. I don't think it's worthwhile trying to play language games, and just answer the obvious question. – Barmar Jan 16 '20 at 17:26
@ritaj yep, bingo. I don't know if the question is transcribed properly or not - it might have been actually calling, say, `readFromDB` and OP chose to transcribe it as `//some asynchronous tasks here` or it might have been something like `localStorage.set("lastValue", i)` which is synchronous. – VLAZ Jan 16 '20 at 17:28

score 2 · Answer 2 · answered Jan 16 '20 at 17:32

You can try with async function awaiting for promise will be resolved.

async function toDo() {
  for (let i = 0; i < 10; i++) {
    await new Promise(res => {
      setTimeout(() => {
        // Running some asynchronous tasks here
        // ...
        console.log('completed task id ' + i)
        res()
      }, 1000)
    })
  }
}

toDo()

Klaycon · Answer 3 · 2020-01-16T17:33:59.820

1

Issues:

setTimeout schedules a function to be run after the specified delay. Note that all setTimeouts are scheduled at the same time, they don't wait for each other. This is what is meant by "asynchronous". Thus, all of your setTimeout calls are with a delay of 1000 ms, and all of them will run consecutively after only 1000ms.
If you really are running other asynchronous tasks in the callback to setTimeout, the console.log will not wait for them unless you tell it to. I can only guess, but make sure you use an await, an async function or another callback.

for (let i = 0; i < 10; i++) {
  setTimeout(function() {
    // Running some asynchronous tasks here
    // ...
    console.log("completed task id " + i);
  }, 1000 * i) //multiply i by 1000 to wait 1,2,3,4... seconds
}

edited Jan 16 '20 at 17:33

answered Jan 16 '20 at 17:25

Klaycon

10,599
18
35

"*all of them will run at the same time after 1000ms.*" no, they will run *one after another*, in sequence, after (close to) 1000ms. They wouldn't be running in parallel. – VLAZ Jan 16 '20 at 17:30
@VLAZ I was referring to the perspective of a viewer, since the delay between outputs will probably be mere nanoseconds, but I can see how the wording implies otherwise. I'll edit the answer. – Klaycon Jan 16 '20 at 17:33

score 1 · Answer 4 · answered Jan 16 '20 at 17:34

1

Maybe another solution will be to use the setInterval function with a 1000ms interval and call setTimeout with 10 seconds waiting times to clear the interval :

const myInterval = setInterval(function() {
  // some async stuff here
  
}, 1000);

setTimeout(function() {
  clearInterval(myInterval)
}, 10000);

answered Jan 16 '20 at 17:34

adrien

115
1
6

This might actually miss the 10th execution. Timers in JS are not precise, so if you say "execute something 1s later" what happens is that it gets executed *as close to* 1s later as the engine can. It might be executed 1.1 or 1.2 seconds later. So, if you add up the overlaps, you may spend 10 seconds re-running the interval running only nine times, at which point it gets cancelled. – VLAZ Jan 16 '20 at 17:38
1

A better way would be to use a counter variable and then have the function call clearTimeout itself when the counter reaches 10. – Barmar Jan 16 '20 at 17:39
@VLAZ Yes indeed, I forgot this detail of a missing iteration, thank you for reminding me. However, I used these two functions to stay in the context of using the basic web api Timer feature, as the problem is initially posed with a setTimeout function – adrien Jan 16 '20 at 17:48
@Barmar It seems more precise indeed – adrien Jan 16 '20 at 17:49

setTimeout does not produce correct async behavior

4 Answers4