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

Add capability to try to detect a possible stack overflow and queue functions the next tick #516

Closed
wants to merge 2 commits into from

Conversation

CrispyConductor
Copy link

There are a number of cases where various iterator functions are used in cases where the iterator isn't actually asynchronous (this is usually when the iterator is conditionally synchronous, but may return immediately in some cases). When iterating over large sets of objects, this can cause unexpected stack overflows.

This commit tries to track estimated stack depth by incrementing a counter each time a tracked function is executed, and clearing the counter each tick. If the counter grows above a threshold, additional functions are queued to execute the next tick automatically.

This also adds the functions async.stackSafe(callback), async.wrapStackSafe(func), and safe variants of existing functions like safeEach(), safeSeries(), etc.

… overflow may be imminent, in the case that an iterator isn't actually asynchronous
@aearly
Copy link
Collaborator

aearly commented Apr 22, 2014

I would 👎 this. The way to fix this is to make your iterators actually async -- synchronous iterators are the only case where I've come across stack overflows.

function myIterator(args, callback) {
  doSomethingSync();
  callback(null);
}

to

function myIterator(args, callback) {
  doSomethingSync();
  async.nextTick(function () {
    callback(null);
  });
}

That being said, async.stackSafe could be useful for people who want a convenience function to wrap their iterators. (Also, if it had tests.)

@CrispyConductor
Copy link
Author

As a user of async, I'd think that the expected behavior of an iterator function, when called with an iterator which may or may not be asynchronous, isn't to generate a stack overflow. Although it's probably more correct for the user to ensure that their callback is asynchronous, in my opinion, it's better to offer some protection against this.

I've added tests and documentation for the relevant functions.

@caolan
Copy link
Owner

caolan commented Apr 28, 2014

I'm :-1 on this too (although we did support this briefly). I see a stack overflow as a good way to detect misuse of the library. It's up to the library consumers to make their functions consistently asynchronous.

@aearly
Copy link
Collaborator

aearly commented Jun 30, 2015

FYI ensureAsync in v1.2.0 and later is a nice way to make an async function "stack-safe".

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.

3 participants