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

async.auto: should subsequent tasks still run if the chain as a whole has been marked as failed? #1023

Closed
bossie opened this issue Feb 9, 2016 · 8 comments · Fixed by #1042
Labels

Comments

@bossie
Copy link

bossie commented Feb 9, 2016

I was under the impression that the behavior of async.auto was that if one of the tasks returned an err, the global callback would be called with this error and all subsequent tasks would not execute. After all, why would they? The global callback has been called so the error has been reported already.

It turns out that only tasks that depend on the faulty task will not run, and the rest will.

I'm not sure if it makes sense for async.auto to report failure by means of a single global callback on one hand, and still run subsequent tasks on the other hand: as far as the client is concerned, the whole thing failed.

@aearly aearly added the question label Feb 9, 2016
@aearly
Copy link
Collaborator

aearly commented Feb 9, 2016

You're right -- only tasks that depend on the faulty task will not run. Any tasks that were in progress when the error occurs will continue, but their results will be ignored.

Is there an issue when an error occurs synchronously as several tasks are being kicked off at the same time?

@bossie
Copy link
Author

bossie commented Feb 11, 2016

I'm afraid I don't understand your question aearly.

The issue I'm facing is as follows. I've got this dependency graph set up with async.auto:
dag

Both "download audio" and "preprocess audio" are long-running tasks. "Download script" is short task, and in this case, it fails immediately. What happens is that the callback passed to async.auto is called with an err, and the entire task is restarted (because the client decides to restart it).

The problem is not really that "download audio" keeps running, but that "preprocess audio" will still run, even though there's no point (in my opinion) because it won't be able to do anything with its results.

The fundamental issue that we're facing is that "preprocess audio" is wasting precious resources and competing with the new run.

I know that I can make "download script" a dependency of "preprocess audio" as well, but 1) I don't really think that makes sense and 2) it means that "preprocess audio" would have to wait for "download script", even though they could run perfectly in parallel.

Any thoughts?

@aearly
Copy link
Collaborator

aearly commented Feb 12, 2016

What version are you using? It sounds like you're describing #988 which was fixed in v1.5.1

@bossie
Copy link
Author

bossie commented Feb 15, 2016

Actually, I was using 1.5.0, so I upgraded to 1.5.1 and the behavior is still wrong. Same for 1.5.2. I then ran the original example in #988 and it seems that bug isn't fixed.

For what it's worth, I'm running Node.js v5.6.0 in Ubuntu 15.04.

@aearly
Copy link
Collaborator

aearly commented Feb 16, 2016

Are you sure you had 1.5.2 installed? There's a specific test case for that bug.

@bossie
Copy link
Author

bossie commented Feb 17, 2016

I know, and that specific test case does work (there is a difference between v1.5.0 and v1.5.2).

However, the example in #988 does not. You might want to try it out for yourself and add it to your test suite.

This is an excerpt from my console session where I:

  1. install async v1.5.0
  2. run the example in auto allows functions to start after error is passed #988
  3. install async v1.5.2
  4. run the example in auto allows functions to start after error is passed #988 again

In both cases, task c is called, even though it shouldn't be because x already returned an error.

bossie@ozymandias:~/WebstormProjects/node-test$ npm install [email protected]
/home/bossie/WebstormProjects/node-test
└── [email protected] 

npm WARN enoent ENOENT: no such file or directory, open '/home/bossie/WebstormProjects/node-test/package.json'
npm WARN node-test No description
npm WARN node-test No repository field.
npm WARN node-test No README data
npm WARN node-test No license field.
bossie@ozymandias:~/WebstormProjects/node-test$ node github.js 
a()
b()
x()
b:Time: 51.264ms
callback(Error: x, {"b":"two"})
x:Time: 101.388ms
a:Time: 201.818ms
c()
c:Time: 0.059ms
bossie@ozymandias:~/WebstormProjects/node-test$ npm install [email protected]
/home/bossie/WebstormProjects/node-test
└── [email protected] 

npm WARN enoent ENOENT: no such file or directory, open '/home/bossie/WebstormProjects/node-test/package.json'
npm WARN node-test No description
npm WARN node-test No repository field.
npm WARN node-test No README data
npm WARN node-test No license field.
bossie@ozymandias:~/WebstormProjects/node-test$ node github.js 
a()
b()
x()
b:Time: 51.204ms
callback(Error: x, {"b":"two"})
x:Time: 101.261ms
a:Time: 202.495ms
c()
c:Time: 0.062ms

@bossie
Copy link
Author

bossie commented Feb 18, 2016

Oh, I noticed that you have to set concurrency to 1 for it to work.

Anyway, the point I'm trying to make is that this "remaining task dumping" makes sense for parallel tasks too. I can't explain it better than I did above, or than the person who did it in #988.

FWIW, I made a gist here that wraps async.auto and seems to do the trick.

@aearly aearly added the bug label Feb 24, 2016
@aearly
Copy link
Collaborator

aearly commented Feb 24, 2016

This looks like a bug. We need a better test case for stopping after an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants