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

fix "RangeError: Maximum call stack size exceeded" #1514

Merged
merged 2 commits into from Feb 16, 2018
Merged

Conversation

smbape
Copy link
Contributor

@smbape smbape commented Feb 14, 2018

eachOfLimit throws a RangeError: Maximum call stack size exceeded when the collection is too big and the iteratee is synchronous.
The usual workaround is to use setImmediate(next) or http://caolan.github.io/async/docs.html#ensureAsync.

I have a data analysis program. The setImmediate(next) workaround made the whole program slow when there are few new data to analyse.

With a small modification, it was possible to make the program lasting from minutes to seconds.

Here is an exemple of time duration with and without setImmediate

const eachOfLimit = require("async/eachOfLimit");

(function() {
    'use strict';

    const collection = new Array(Math.pow(1024, 2));
    for (let i = 0, len = collection.length; i < len; i++) {
        collection[i] = i + 1;
    }

    const timerInit = Date.now();
    eachOfLimit(collection, 1, (value, i, next) => {
        next();
    }, err => {
        console.log("sync", Date.now() - timerInit, "ms");
    });
}());

(function() {
    'use strict';

    const collection = new Array(Math.pow(1024, 2));
    for (let i = 0, len = collection.length; i < len; i++) {
        collection[i] = i + 1;
    }

    const timerInit = Date.now();
    eachOfLimit(collection, 1, (value, i, next) => {
        setImmediate(next);
    }, err => {
        console.log("async", Date.now() - timerInit, "ms");
    });
}());

Copy link
Collaborator

@hargasinski hargasinski left a comment

Thanks for the PR! It looks good, except for a couple of small changes. We implemented something similar for queues in #1334, so I don't see an issue in adding this to eachOfLimit (and the collection methods that use eachOfLimit internally).

if (err) throw err;
expect(count).to.equal(1024 * 1024);
});
setTimeout(done, 25);
Copy link
Collaborator

@hargasinski hargasinski Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you call done inside of the final callback instead?

@@ -274,6 +274,18 @@ describe("eachOf", function() {
setTimeout(done, 25);
});

it('forEachOfLimit 1024 * 1024 synchronous call', function(done) {
Copy link
Collaborator

@hargasinski hargasinski Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor, but we already have a forEachOfLimit synchronous test. Could you rename this to something like forEachOfLimit no stackoverflow or forEachOfLimit no call stack size exceed error just to differentiate between the two?

Copy link
Collaborator

@hargasinski hargasinski Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a similar test for forEachOf and forEachOfSeries to make sure the behaviour is consistent?

…it no call stack size exceed error'

done called inside of the final callback of 'forEachOfLimit no call stack size exceed error'
added 'forEachOf no call stack size exceed error' and 'forEachOfSeries no call stack size exceed error' tests
@hargasinski
Copy link
Collaborator

@hargasinski hargasinski commented Feb 15, 2018

I don't know why the build initially failed, but it appears to be working now. LGTM.

aearly
aearly approved these changes Feb 16, 2018
Copy link
Collaborator

@aearly aearly left a comment

A clever, simple solution to a common problem.

@megawac
Copy link
Collaborator

@megawac megawac commented Feb 16, 2018

👍 I was originally going to comment that this might be a misuse of async, but to be honest I've hit this case in my own personal use enough times to think its justified. That this solution is so simple is just icing on the cake.

@megawac megawac merged commit f5d86b8 into caolan:master Feb 16, 2018
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants