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

detectSeries breaks when iterating over large arrays #1293

Closed
asquared opened this issue Oct 3, 2016 · 4 comments
Closed

detectSeries breaks when iterating over large arrays #1293

asquared opened this issue Oct 3, 2016 · 4 comments
Labels

Comments

@asquared
Copy link

asquared commented Oct 3, 2016

Async 2.0.1, Node v0.10.29.

detectSeries on a large array causes a stack overflow even if the callbacks are called via setImmediate. Test case:

const async = require('async');

var arr = [];
for (var i = 0; i < 10000; i++) {
        arr.push(i);
}

// this should print 0, then print done, because the detection succeeds on the
// first element of arr. It does, but then it causes a stack overflow
async.detectSeries(arr, function(data, cb) {
        console.log(data);
        setImmediate(cb, null, true);
        // cb(null, true) is definitely expected to cause a stack overflow
        // but setImmediate does not appear to change the situation
}, function(err, result) {
        console.log("done");
});

What did you expect to happen?

0
done

What was the actual result?

0
done

.../node_modules/async/dist/async.js:843
        return function () {
                        ^
RangeError: Maximum call stack size exceeded

The bug appears to be tied to the size of arr. arr with 1000 items does not trigger the issue on my system.

@megawac megawac added the bug label Oct 3, 2016
@megawac
Copy link
Collaborator

megawac commented Oct 3, 2016

This is a bug in the handler of the case of a resolved detect callback. When detect has returned true we essentially just replace your iteratee with function(cb) {cb()} which is synchronous.

#1064 would fix this

@aearly
Copy link
Collaborator

aearly commented Oct 4, 2016

Is there a more short term fix that doesn't involve #1064? This seems pretty major - surprised it didn't pop up sooner.

@megawac
Copy link
Collaborator

megawac commented Oct 4, 2016

Not sure, thinking maybe an internal only #1064 is called for though

@asquared
Copy link
Author

asquared commented Oct 4, 2016 via email

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

3 participants