async.each callback could be called more than once when array is modified

[mingqi]

var async = require('async')

inputs = [1,2]
async.each(inputs,
    function (item, callback) {
        setTimeout(function(){callback()}, 1000)
    },
    function(err){
        console.log("async.each done")  
    })

inputs.splice(1,1)

/* output is:
async.each done
async.each done
*/

the problem is async use completed >= arr.length to check if call callback. however arr.length is less than async.each was called.

[aearly]

Modifying an array while iterating over it is not recommended --- don't do that.

If you want to submit a patch to fix this, that would be great. Otherwise, if you do need to modify the array, just clone it before you pass it to an async method.

[lysdexia]

I'm new to node/async and made what is probably a very newbie mistake while using waterfall in the same way as the example above:

http://stackoverflow.com/questions/24289629/node-js-async-waterfall-more-than-one-callback-per-function

The weirdness to me is: it works for a while. :-D What is going on in the background that seemingly allows the multiple callback() calls to work?

[amv]

@lysdexia this issue does not deal with multiple callback calls within one iterator. The "series" function referenced here should already defend properly against multiple callbacks (but your code might otherwise do something wonky). This issue is only related to the iterated array being altered in place while the iterator is still working. If you alter the array to contain as many items as it had before, the "series" function works by accident.

The "each" function on the other hand does NOT defend against multiple callback calls. I've just made one issue for that with #559 which now references to an older pull request discussing the same thing.

As a reference - internally async has two "basic" iterators. "each" - which runs functions in parallel, and "series" - which runs functions one after another. Most of the other functions are implemented by using one of these or combining them. Both of these functions might behave in seemingly mysterious ways if A) the array given is altered in place while the iteration is still ongoing and B) if the callbacks of the iterator function are called more than one time (or less than one time). This means also the other functions that use these show the mysterious behaviour.

I have gotten the feeling that B is not seen as such a big deal as you are obviously using the system in the wrong way if you do not call the iterator callback functions once and only once.

I however think that A (which this issue is about and what my pull request addresses) is actually very hard for the programmer to infer as a requirement when using async - and thus defending against it would be an important change.

[aearly]

This was fixed as a side effect of other changes.

[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.

[aearly]

Modifying the array causes undefined behavior. If you need to modify the array, use async.queue.