Stack overflow in async.queue

[PaulMougel]

When too many tasks are added synchronously to an async.queue, the queue will then process all of them in the same stack, which causes a stack overflow.

Here is a gist example.

---

Here is a failing test case (add this to test.test-async.js):

exports['queue big stack'] = function (test) {
    var q = async.queue(function (task, callback) {
        callback();
    }, 1);

    var i;
    for (i = 0 ; i < 10000 ; i++) q.push(1);
    q.drain = test.done;
};

that can be fixed by replacing async.js:725:

q.process()

by

async.setImmediate(q.process);

(related to issue #117) But this unfortunately breaks the queue events test, as the events are then emitted in the wrong order:

AssertionError:
[ 'saturated',
  'process foo',
  'process bar',
  'process zoo',
  'foo cb',
  'bar cb',
  'zoo cb',
  'process poo',
  'empty',
  'process moo',
  'poo cb',
  'moo cb',
  'drain' ]
deepEqual
[ 'saturated',
  'process foo',
  'process bar',
  'process zoo',
  'foo cb',
  'process poo',
  'bar cb',
  'empty',
  'process moo',
  'zoo cb',
  'poo cb',
  'moo cb',
  'drain' ]
    at Object.same (/root/workspace/async/node_modules/nodeunit/lib/types.js:83:39)
    at Object.q.drain (/root/workspace/async/test/test-async.js:2425:14)
    at next (/root/workspace/async/lib/async.js:723:31)
    at Object._onImmediate (/root/workspace/async/lib/async.js:24:16)
    at processImmediate [as _immediateCallback] (timers.js:330:15)

Does anyone have a real fix for this issue?

[pkt-zer0]

Isn't the problem here that you're invoking the callback synchronously? (which you shouldn't do, as per Effective JS item 67)

[PaulMougel]

Indeed, you are correct!

[ausey00]

Sorry to resurrect something thats been closed, but I'm having a similar issue and can't understand why the test file is incorrect?

What is the correct way of calling the callback?

Again, sorry to be a pain

[PaulMougel]

The basic takeaway is that you should not call synchronously a callback. If a function takes a callback, it means that it should call it on another tick. If your code is purely synchronous, you should use setTimeout() or process.nextTick() to asynchronously call the callback. As suggested, you should read item 67 of Effective JS, this article or this SE question.

[ausey00]

Paul, Thanks for getting back to me, and thanks for clarifying! I must have some not-so-obvious recursive function call somewhere in my code as I'm still struggling with the same error.

Austin

[davisford]

I fully understand the premise and the recommendation, but this does put the onus on the user of the library for something that just isn't all that well-enough understood. Common knowledge about item 67 of Effective JS should prevail in an ideal world, but the amount of code written that short-circuits the logic of a function due to preconditions is going to trump that every time IMHO, e.g.

var process = function (data, cb) {
   if (!data || /* data does not pass some sanity test */) { 
    cb();
   } else {
     // continue with async call
   }
}
var queue = async.queue(process, 1);
var i;
for (i = 0; i < 10000; i++) { 
  queue.push(i);
}
// Stack Overflow

It is understood here that the solution is

var process = function (data, cb) {
   if (!data || /* data does not pass some sanity test */) { 
     async.setImmediate(cb());  // process.nextTick() or similar
   } else {
      // continue with async call
   }
}

Think about the number of usages in the wild for async.queue and async.forEachSeries / async.eachSeries, and the number of lines of code that would need to be changed to resolve this kind of issue -- whereas it is a simple fix to guard against it in the library itself, and likely generates minimal performance penalty.

[aearly]

Generating minimal performance penatly is the real issue. While the library could easily defer all callbacks inside queue, eachSeries, et al, and prevent all stack overflows, that's a 4 ms penalty per callback in the browser. Even process.nextTick()s in node add up if you do a lot of them.

I am drafting a couple solutions to the problem, I will have a proposal in a few days or so.