Return array of errors in parallel forEach (keeping compatibility)

[fmenezes]

Keeping compatibility with the callback(err[, errs]), return first error and array of errors.

[aearly]

Can you add a test case for this?

[fmenezes]

Done.

[aearly]

👍 Now it's just left for @caolan to merge.

[andreineculau]

This PR maintains compatibility on the level of async.each but breaks uniformity with other async.* functions, something which is total no-no for me.

1. I'd very much like a generic way of telling async "don't stop on first error in a parallel flow, but collect all errors"
2. I'm curious, what is your use-case? I'd imagine that the next steps in your code, depend on all callbacks to "resolve", right?

Also see: #334

UPDATE By uniformity, I mean that I'll always think first error ends up errroing the flow. And then I call async.each with 1 million items, the first callback errors, but with your PR, it will still continue eating cycles.

[caolan]

I agree with @andreineculau - this kind of error handling seems like a whole other branch of async functions atm (partly why I'm experimenting with Highlandjs, which lets you compose this kind of behaviour).

[fmenezes]

My case is that, I have an array of items and I'm calling an external service.

When the error comes out, I cannot know which item has executed or not.

Look at this example:

var async = require('async');
var executed = [];
async.forEach([1, 2, 3], function(item, callback) {
  console.log(item);
  var ok = (item != 2);
  if (ok) {
    executed.push(item);
  }
  callback(!ok ? new Error('Bad thing') : null);
}, function (e) {
  console.log('end', e, executed);
});

the output:

1
2
end [Error: Bad thing] [ 1 ]
3

Async cotinues executing no matter the error pops out.

I agree with you @andreineculau I doesn't solve the problem at all, it is just a "workaround".

I believe a better solution (in architectural point of view at least) would be something like another version of async, lets think about a 'collect version':

var async = require('async');
var executed = [];
async.collect().forEach([1, 2, 3], function(item, callback) {
  console.log(item);
  var ok = (item != 2);
  if (ok) {
    executed.push(item);
  }
  callback(!ok ? new Error('Bad thing') : null);
}, function (e) {
  console.log('end', e, executed);
});

the ideal output would be:

1
2
3
end [ [Error: Bad thing] ] [ 1, 3 ]

UPDATE: Updated code

[fmenezes]

Example at fmenezes@c83a749

[andreineculau]

@fmenezes knowing from which item the error bloomed is more than fair, but I could go for

keepItemOnErr = (iterator) ->
  (item, next) ->
    iterator item, (err, args...) ->
      if err?
        return next [item, err]
        ###
        # alternative version if err is preferred to be instanceof Error
        err.asyncItem = item
        return next err
        ###
      next  null, args...

arr = [1, 2, 3]
iterator = (item, next) ->
  next new Error('it takes 2 to tango')  if item isnt 2
  next null, item
callback = ([item, err], results) ->
  item == 2 and err.message == 'it takes 2 to tango'
async.each arr, keepItemOnErr(iterator), callback

PS: sorry, had to go for coffeescript, for a bit of dramatic effect :)

[fmenezes]

Yes, I can always create an iterator to solve my issues, what is exactly what I do today, but the motivation behind this pull request was the question of how to deal with errors in the core each method, since it is parallel stopping at the first error (what is not actually true) seems to be "different".

[fmenezes]

But good point, returning the item within it's error is a good feature.

[aearly]

I would also like to point out that async.queue might be helpful if you need to run tasks in parallel, but also have granular handling of results.

[fmenezes]

Yes, the async.queue is awesome, but in this particular case the "final callback" is much more convenient than the drain method.

Like @andreineculau showed we also could do something like a specific iterator or method.

function myLoop(arr, iterator, callback) {
  var errors = [];
  async.forEach(arr, function(item, callbackIterator) {
    iterator(item, function (e) {
      if (e) {
        errors.push(e);
      }
      callbackIterator();
    });
  }, function () {
    if(errors.length === 0) {
      errors = null;
    }
    callback(errors);
  });
}

myLoop([1, 2, 3], function (item, callback) {
  console.log(item);
  callback((item == 2) ? new Error('Bad thing') : null);
}, function (e) {
  console.log(e);
});

But I believe async can handle this, without breaking compatibility.

[aearly]

I really dislike the global collecting state here -- it will cause issues with multiple collecting functions running in parallel. Closing in favor of #349.