Async does not protect against array modifications during the function's runtime

[bradens]

When you have an asynchronous call eg. async.each, and during that runtime, the array which is passed in has modifications to it, then it may never finish, or will call the callback twice.

Example:

async = require "async" 
arr = [1, 2, 3]  
async.each arr, ((i, cb) -> console.log "i"; setImmediate(cb)), (err) -> console.log "done" 
arr.push(4)

This example loops through the 3 original array elements, and prints i but then never calls the callback, because in async.each it's doing:

if (completed >= arr.length) {
  callback(null);
}

When looking at the async code, it's doing a comparison against arr.length which can change...would it not be better to store the original array length and do a comparison against that, ensuring that the completed callback will be called?

Fiddle:
http://jsfiddle.net/4ysKX/1/

[aearly]

You're right -- so don't modify it. Clone your array before you pass it to an async function if you need to change it later.

[kenpratt]

@aearly Yes, that's a work-around, but the underlying issue here is that it's a very easy mistake to make, and a very hard issue to debug, so IMHO it should be fixed in the core library. This has bitten me twice now on two different projects.

It would be hard to protect against any array modification without cloning the input, for example a synchronous modification in the iterator function, but protecting against asynchronous modification unrelated to the iteration should be straightforward even without clone -- really just not assuming arr.length will stay constant after spinning up the async jobs (saving the length in a variable before the iteration).

(I work with @bradens -- one of us will submit a PR with tests if that's welcome).

[aearly]

A PR with tests would be welcome, but it's ultimately up to @caolan to merge it.

The downside to doing this is the extra overhead of the array copying. If you have a large array, or are calling async.each et al many times, it will be slower and use more memory.

[kenpratt]

I agree that an array copy would be too much overhead. I'm suggesting that since the initial iteration of the array is synchronous, the original array length could be saved to check against later. That way, the number of callback calls will match the number of iterator calls, even if the array is changed in the meantime. This should be easy and zero overhead for async.each, but I haven't looked at the other functions yet. It would be nice if all the parallel functions had consistent behaviour in these cases.

[aearly]

This is a duplicate of #557.

[ORESoftware]

Did async end up disallowing array modifications after the fact? I have a use case where it would be nice to modify the original array after the fact, so as to iterate on more elements than initially.