Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Reflect" behavior for collecting errors/results and continuing. #942

Closed
aearly opened this issue Oct 25, 2015 · 16 comments
Closed

"Reflect" behavior for collecting errors/results and continuing. #942

aearly opened this issue Oct 25, 2015 · 16 comments
Labels
Milestone

Comments

@aearly
Copy link
Collaborator

aearly commented Oct 25, 2015

Related to #349 #334 #531 .

There have been many requests for a behavior where when an iterator calls back with an error, the error is collected, and execution continues. There have been a couple attempts at this feature, but the implementations fall short.

In theory, this would be applied to just about any async function, map, series, parallel, forEachSeries, etc.. I'd like to support this broadly, but without having to create duplicate implementations for each function. Something we can wrap existing functions with, without having to modify the implementations.

We also need to settle on what the name is for this type of behavior. These have been proposed:

  • mapCollect / parallelCollect / eachSeriesCollect
  • mapContinue / parallelContinue / eachSeriesContinue
  • mapAnyway / parallelAnyway / eachSeriesAnyway
  • mapMaybe / parallelMaybe / eachSeriesMaybe
  • mapResume / parallelResume / eachSeriesResume

Lastly -- how should the collected results be passed to the final callback?

  • callback(null, {errors: [], results: []])
  • callback(null, [Error, results, results, Error, results..])
  • callback(null, [{err: null, result: results0}, {err: Error, result: null}, ...])
  • callback(null, results, errors)
@megawac
Copy link
Collaborator

megawac commented Oct 26, 2015

I would suggest callback(null, results, errors) or callback(null, [Error, obj, obj, Error, etc..])

@tommy31
Copy link

tommy31 commented Dec 2, 2015

Any update of this?

@aearly
Copy link
Collaborator Author

aearly commented Dec 2, 2015

Sorry, we've been too busy to make any progress on this. We also haven't decided on the naming pattern, and haven't thought up a clean way to add it to the library.

@jtwebman
Copy link
Contributor

Actually bluebird is switching to a map with a reflect wrapper around each. Maybe that would be easier? So reflect would wrap each of the callbacks and always return successful. The output would then have null as error and a list of reflect objects that with have a function hasValue() method that returns true if the reflect .value property has a value (ie. didn't error) and if it did error then the .error property will have the error.

Here is what the code would look like.

async.series([
  async.reflect(function(callback){
    // do some stuff ...
    callback(null, 'one');
  }),
  async.reflect(function(callback){
    // do some more stuff but error ...
    callback(new Error('bad stuff happened'));
  }),
  async.reflect(function(callback){
    // do some more stuff ...
    callback(null, 'two');
  })
],
// optional callback
function(err, results){
  // values
  // results[0].value = 'one'
  // results[1].error = Error('bad stuff happened')
  // results[2].value = 'two'
});

@jtwebman
Copy link
Contributor

We can add a reflectAll function as well that can take a list of functions with callbacks and wrap them all in async.reflect like this:

let tasks = [
  function(callback){
    setTimeout(function(){
        callback(null, 'one');
    }, 200);
  },
  function(callback){
    // do some more stuff but error ...
    callback(new Error('bad stuff happened'));
  }
  function(callback){
    setTimeout(function(){
      callback(null, 'two');
    }, 100);
  }
];

async.parallel(async.reflectAll(tasks),
// optional callback
function(err, results){
  // values
  // results[0].value = 'one'
  // results[1].error = Error('bad stuff happened')
  // results[2].value = 'two'
});

@aearly
Copy link
Collaborator Author

aearly commented Feb 2, 2016

@jtwebman I like your approach. It means we don't have to create new methods for each flavor of async function, leaving it to the user to use it when continuing is desired. Also, +1 for the consistency with Bluebird's direction.

@danielshir
Copy link

Great idea, although the only thing that irks me is wrapping the tasks themselves.
The code already has major boilerplate that it requires, so how about using a sub object?
I write most of my code inline (e.g. not using a "tasks" var) so I'd prefer it this way.

Say something like :

let tasks = [
  function(callback){
    setTimeout(function(){
        callback(null, 'one');
    }, 200);
  },
  function(callback){
    // do some more stuff but error ...
    callback(new Error('bad stuff happened'));
  }
  function(callback){
    setTimeout(function(){
      callback(null, 'two');
    }, 100);
  }
];

async.reflect().parallel(tasks),
// optional callback
function(err, results){
  // values
  // results[0].value = 'one'
  // results[1].error = Error('bad stuff happened')
  // results[2].value = 'two'
});

@jtwebman
Copy link
Contributor

@danielshir How is that different then this:

let tasks = [
  function(callback){
    setTimeout(function(){
        callback(null, 'one');
    }, 200);
  },
  function(callback){
    // do some more stuff but error ...
    callback(new Error('bad stuff happened'));
  }
  function(callback){
    setTimeout(function(){
      callback(null, 'two');
    }, 100);
  }
];

async.parallel(async.reflectAll(tasks),
// optional callback
function(err, results){
  // values
  // results[0].value = 'one'
  // results[1].error = Error('bad stuff happened')
  // results[2].value = 'two'
});

This way is more explicit without having to hide some magic under some object that treats parallel or any other method different plus not all methods make sense to use reflect.

jtwebman added a commit to jtwebman/async that referenced this issue Feb 18, 2016
…a object with error or value property set.

This is one way to solve issue caolan#942.
@danielshir
Copy link

Well, if that's your writing style then I guess it looks the same.
But, what if you write your functions inline? As I assume most people do.

It's a bit messy for my taste since you're adding another scope (more parentheses) -

async.parallel(async.reflectAll([function(callback){
    setTimeout(function(){
        callback(null, 'one');
    }, 200);
  },
  function(callback){
    // do some more stuff but error ...
    callback(new Error('bad stuff happened'));
  }
  function(callback){
    setTimeout(function(){
      callback(null, 'two');
    }, 100);
}]
),
// optional callback
function(err, results){
  // values
  // results[0].value = 'one'
  // results[1].error = Error('bad stuff happened')
  // results[2].value = 'two'
});

@jtwebman
Copy link
Contributor

@danielshir Then you can write it like this if you want:

async.parallel([function(callback){
    setTimeout(function(){
        callback(null, 'one');
    }, 200);
  },
  function(callback){
    // do some more stuff but error ...
    callback(new Error('bad stuff happened'));
  }
  function(callback){
    setTimeout(function(){
      callback(null, 'two');
    }, 100);
}].map(async.reflect),
// optional callback
function(err, results){
  // values
  // results[0].value = 'one'
  // results[1].error = Error('bad stuff happened')
  // results[2].value = 'two'
});

Leaving these just functions keeps it very simple and doesn't spread the code to do this all over the place.

@danielshir
Copy link

@jtwebman I didn't think of that and that actually looks way better!
The reflect call is right near the finishing callback which will remind the programmer why results look like they do 👍

@enrichz
Copy link

enrichz commented Mar 7, 2016

I'm very interested in this feature, thanks for your work guys. +1

@mahlu
Copy link

mahlu commented Mar 17, 2016

+1

@aearly aearly changed the title "Collect"/"Resume" Behavior "Reflect" behavior for collecting errors/results and continuing. Mar 21, 2016
@akayami
Copy link

akayami commented Mar 22, 2016

Great feature for me, but I do not see this in 2.0.0-rc.1 - Any hope it will be ready soon ?

@aearly
Copy link
Collaborator Author

aearly commented Mar 22, 2016

We're waiting for #1012 to be completed. No ETA. It might wait until 2.1.

@aearly
Copy link
Collaborator Author

aearly commented Apr 4, 2016

Closed via #1095 !

@aearly aearly closed this as completed Apr 4, 2016
ipyramiddev added a commit to ipyramiddev/async-300 that referenced this issue Dec 10, 2022
How to continue *parallel* processing in case of a failing task seems to be a frequently asked question. It was mentioned in several issues:

- caolan/async#334
- caolan/async#675
- caolan/async#798
- caolan/async#942

Adding a hint to the documentation might help users of the library to find out about the answer to this question.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants