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

Detecting dead-locks in async.auto() and throwing an Error #663

Merged
merged 3 commits into from
May 19, 2015

Conversation

Mickael-van-der-Beek
Copy link

This pull request adds dead-lock detection in async.auto() due to dependencies and thus fixes #263.

The three dead-lock cases that were handled and for which test cases were added are:

    1. inexistant dependency; e.g:
async.auto({
    task1: ['noexist', function(callback, results){
        callback(null, 'task1');
    }]
});
    1. cyclic dependencies; e.g:
async.auto({
    task1: ['task2', function(callback, results){
        callback(null, 'task1');
    }],
    task2: ['task1', function(callback, results){
        callback(null, 'task2');
    }]
});
    1. self as dependency which is a subcase of 2.; e.g:
async.auto({
    task1: ['task1', function(callback, results){
        callback(null, 'task1');
    }]
});

If any of these cases are encountered, a verbose Error() is thrown.

It is possible to improve the performance a little bit further by embedding these detections inside the ready() method because we already loop through the requires array during the Array.reduce() call:

Mickael-van-der-Beek/async@2410077e26d5429ac45533438f008706dcb989f7/lib/async.js#L492

but I felt that it would be a dirtier solution.
If you prefer the faster solution though, just ask and I'll create a new pull-request for it.

@aearly
Copy link
Collaborator

aearly commented Nov 18, 2014

👍 Cool stuff. Now we just need to wait for @caolan ....

@beaugunderson
Copy link
Collaborator

@Mickael-van-der-Beek Can you fix the conflicts so this merges cleanly?

@aearly I'm happy to merge too, caolan gave me contributor access when I asked if he needed help last month. You should ask as well (assuming you'd want it), I'm fairly sure he'd oblige given that you already seem to be doing the majority of the maintenance. :)

@Mickael-van-der-Beek
Copy link
Author

Ok, I merged my fork's "dead-locks" branch with caoloan's upstream master branch.
The build seems to pass.

@beaugunderson Would it be possible to merge ?

@aearly
Copy link
Collaborator

aearly commented Dec 11, 2014

@beaugunderson If you say so I'll ask. :) Getting push access to such a widely used library is a huge responsibility, though...

@Mickael-van-der-Beek
Copy link
Author

Is there anyway for this to be merged ?

If nobody's got time and trusts me enough to give me contributor access, I could do it myself.

@aearly
Copy link
Collaborator

aearly commented Mar 26, 2015

We have to resurrect @caolan from his slumber.

aearly added a commit that referenced this pull request May 19, 2015
Detecting dead-locks in async.auto() and throwing an Error
@aearly aearly merged commit 9206415 into caolan:master May 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enhance auto(…) to detect missing dependencies
3 participants