Series does not defend against multiple callback calls

[amv]

var async = require('async')

async.series(
    [function (callback) {
        callback();
        callback();
    }],
    function(err){
        console.log("async.series done")  
    })

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

The bug also causes last functions in the series to not be executed if preceding functions call the callback multiple times.

We could fix this by using the only_once() wrapper but I am a bit hesitant to do this as only_once actually throws an Error, which might cause some existing code to fail.

@caolan can you give your opinion on wether we should create a "silent_only_once" which would allow iterables to call the callback multiple times without throwing an Error, but would deactivate the callback after the first call, guard the actual series execution against multiple calls?

[aearly]

Related to #535 . I don't think a consensus was reached on whether to defend against this...

[amv]

@aearly thank you for pointing to the previous discussion about this.

I personally like the idea of making async strict by default. However given the reach of this library I would make the transition gradually:

Step 1: The soft fix

1. Implement basic guarding of execution logic (wrap "each" iterator in "warning_only_once") and make sure that if callbacks are called more than once, async executes a "console.warn" warning about faulty code. This should keep most code which is not horribly wrong still operational and inform users of their misdeeds.
2. Implement an optional mode "async.strict_each = 1" which is not on by default. Setting this on would make multiple calls to "each" callbacks die as "only_once" currently does.
3. Encourage people to use the "async.strict_each = 1" mode with all new code in the module documentation and examples. Tell people that this mode will be the default in future releases.
4. Make it possible to suppress these warnings with an environment variable "NODE_ASYNC_DISABLE_STRICT_EACH_WARNINGS = 1". The reason to use an environment variable instead of a mode in the async object is to make the change affect also faulty usage of async in the used code dependencies.

Step 2: The hard fix

1. Wait for one year so that developers have adequate time to get a hold of the issue.
2. Make the "async.strict_each = 1" behaviour the default behaviour and remove option to set it to 0.
3. Make it possible to set an environment variable "NODE_ASYNC_DISABLE_STRICT_EACH = 1" which makes the process act like it was still in Step 1. Inform users that it is strongly suggested to ignore the option unless it is absolutely necessary to keep old code that can not be refactored working. Tell the users that this option will go away in the future releases.

Step 3: Clean table

1. Wait for an another year so that developers have adequate time to get a hold of the issue.
2. Remove the environment variable options. Inform users that if they still need to maintain broken code functional, they can bind to an older release.
3. Cross fingers and hope that by then the NPM ecosystem has found a good way to force the exact version of a dependency also to dependencies of dependencies that are more lax with their required dependencies.
4. Also hope that somebody has by then found a better way to express the above dependency problem without using the word dependency so many times.

If you want to do less work and be a bit more cruel, you can maybe go to Step 2 straight away. However I would sadly have to personally voice my opinion against it. This is due to the NPM dependency problem outlined above and the fact that due to it, even careful developers who have bound their code to exact NPM version numbers might get sudden and very hard to debug error crashes. This is because they might have used some version of some library which is more lax with the version of async which they depend on, while still containing code that works by accident given the current state of things.

I would definitely love to get also @caolan involved in this discussion.

[amv]

Actually it seems that NPM already does have a "npm shrinkwrap" feature which allows the user to bind specific versions recursively for the whole dependency tree.. Which is great!

I have no idea how I have been able to miss this before, but it pretty much means that Step 2 could just be jumped over as the developers have the option to fix the async version of misbehaving dependencies after they have identified them during Step 1.

[aearly]

While I'm still on the fence as to whether async should guard against improper async functions, guarding against this and throwing an error when an iterator calls-back twice would be a good candidate feature for a 1.0 release. Skip straight to Step 3. If we're going to do it, do it soon.

[smtlaissezfaire]

This bug seems like it's already fixed (as of HEAD - 9939951).

Check out this behavior with async.series:

So it seems that async.series now guards against multiple calls to the same callback.

async.waterfall, on the other hand, allows callbacks to be called multiple times, but will only call the final (error or done) callback once.

This behavior seems inconsistent and should be documented, if intended.

How would one now use the old behavior of async.series if code depended on multiple calls to the same callback? Consider this code:

https://gist.github.com/smtlaissezfaire/016b54bc906a2dc7df87

[amv]

@smtlaissezfaire I agree that this is inconsistent, although I personally would view the ability to call callbacks multiple times a bug in itself.

However I think it would be best to close this issue if the original "series" problem has been fixed in a new release (I believe this has indeed happened based on discussion in other threads), and create a new issue for this so that we can discuss the changes required to fix this in isolation.

Could you post a fresh bug issue about this, then post a comment here linking to that issue? Specifically I would like to see the title not containing the word "series", as it links to both the functions that have "series" in their names, and an internal series structure used within the library to implement these. The issue you describe does not relate to either of them, so it would be better to have a new, clean issue.

We could then close this one and keep the issue space in a bit more manageable state :)

[amv]

The most recent version throws an error with the code attached in the first comment:

#  temp_test_async  node test.js 
async.series done

/Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:38
            if (called) throw new Error("Callback was already called.");
                              ^
Error: Callback was already called.
    at /Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:38:31
    at /Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:705:17
    at /Users/amv/Desktop/temp_test_async/test.js:6:9
    at /Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:699:13
    at iterate (/Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:256:13)
    at Object.async.forEachOfSeries.async.eachOfSeries (/Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:275:9)
    at Object.async.series (/Users/amv/Desktop/temp_test_async/node_modules/async/lib/async.js:698:15)
    at Object.<anonymous> (/Users/amv/Desktop/temp_test_async/test.js:3:7)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)

I would consider this bug fixed.

[aearly]

@smtlaissezfaire waterfall had a specific test case geared to reproduce that kind of behavior -- running through the remaining tasks twice if a callback is called twice. I'm actually going to remove it in 2.x -- only one callback per iteration across the board.

The example in your gist could be changed to separate the creation and adding of the event handler from the validation and submitting steps. async isn't needed at all for the first two steps.

[aearly]

Series should defend against multiple callbacks now.