Risk of stack overflow in eachSeries/forEachSeries

[mkw]

The eachSeries/forEachSeries implementation will create a stack that is at least 3x the height of the list passed in. There are probably better ways to fix this, but the easiest is to simply call process.nextTick(iterator); when looping.

i.e. Change:

    async.eachSeries = function (arr, iterator, callback) {
        callback = callback || function () {};
        if (!arr.length) {
            return callback();
        }
        var completed = 0;
        var iterate = function () {
            iterator(arr[completed], function (err) {
                if (err) {
                    callback(err);
                    callback = function () {};
                }
                else {
                    completed += 1;
                    if (completed >= arr.length) {
                        callback();
                    }
                    else {
                        iterate();
                    }
                }
            });
        };
        iterate();
    };
    async.forEachSeries = async.eachSeries;

To:

    async.eachSeries = function (arr, iterator, callback) {
        callback = callback || function () {};
        if (!arr.length) {
            return callback();
        }
        var completed = 0;
        var iterate = function () {
            iterator(arr[completed], function (err) {
                if (err) {
                    callback(err);
                    callback = function () {};
                }
                else {
                    completed += 1;
                    if (completed >= arr.length) {
                        callback();
                    }
                    else {
                        process.nextTick(iterate);
                    }
                }
            });
        };
        iterate();
    };
    async.forEachSeries = async.eachSeries;

[aearly]

This happens when you use an iterator function that has a synchronous callback, and use an array with lots of items. Just make sure your iterator is actually asynchronous with a process.nextTick and you should be good to go, no need to change async.

[mkw]

That's in face exactly what I did, but it was a surprise. Much more than 90% of the time, when I use async, all the functions I have it call will be asynchronous.

But, occasionally, usually for dealing with pre-conditions, I short circuit and just call the callback. I haven't studied enough of the code to be sure, but in all other cases, nothing bad happens. When using eachSeries, it causes a stack overflow, and given that no stack is produced for those, this can be painfully difficult to find. In fact, though I found this through inspection, where it appears in our code makes it somewhat likely to be the cause of a rare stack overflow that I'd pretty much written off finding.

Granted, it does seem odd to modify async for this, but due to the difficulty of finding and fixing stack overflows, it also seems unusually dangerous.

[baslr]

With

  var a, async, i;

  async = require('async');

  a = (function() {
    var _i, _results;
    _results = [];
    for (i = _i = 1; _i <= 10000000; i = ++_i) {
      _results.push(i);
    }
    return _results;
  })();

  async.eachSeries(a, function(i, cb) {
    console.log(i);
    return cb();
  }, function() {
    return console.log('done');
  });

I get

998
999
1000
1001
(node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
(node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
(node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
(node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.

When I apply #588 to master.

setImmediate(iterate);

works

[born2net]

confirm that this fixes the issue, please push to main build

[aearly]

See #296, #413 and #516. Stack overflow == doin' it wrong.

[mkw]

I don't disagree that this is a bug in how using async, but the problem is that it is woefully difficult to find these errors. In our case, someone did something dumb like:

if (extremelyRareGuardClause) {
  cb();
  return;
}
myAsyncOp(function() { cb;} );

Even if we had automated tests that could prove both paths of the branch were taken (and I confess we did not), we would have had a passing build provided the list of test elements was not so long that it caused the stack overflow. The only case that caused the problem was a very long list that happened to hit the rare guard close enough times in a row to blow up the stack. Worse, because order wasn't guaranteed, running exactly the same case more than once wouldn't cause consistent failure!

I would love to "use" stack overflows as a way to debug this, but they give no useful context information whatsoever -- e.g. this is a real error message:

uncaughtException: Maximum call stack size exceeded date=Tue Aug 12 2014 13:32:10 GMT-0500 (CDT), pid=14, uid=55550, gid=55550, cwd=/app, execPath=/app/vendor/node/bin/node, version=v0.10.28, argv=[/app/vendor/node/bin/node, /app/web.js], rss=281776128, heapTotal=245461832, heapUsed=190811968, loadavg=[0.45947265625, 0.24609375, 0.185546875], uptime=11548.720394642, trace=[], stack=undefined

Nothing in that tells us anything useful, and I know of no way to get anything useful.

The bug that caused this existed for over a year before we even hit the stack overflow case and was spotted totally by chance in a later code read more than 6 months after it started a slow climb in crash frequency due to it.

I'm still relatively new to node, and maybe I'm an idiot for not knowing how to force it to dump the stack when it overflows (or maybe extract it from a core file), but that seems like a really painful way to debug this.

[born2net]

please insert this fix to latest build so I can update to latest version of async

[aearly]

@born2net Fix your iterator function, and then you can upgrade. That will be quicker than waiting for a solution from async, if it comes at all.

[aearly]

Closing based on discussion in #696