Fix for Stack Overflow in Whilst and Until

[Radagaisus]

Hey,

var i = 0
var test = function() { return true; }
var iter = function(c) {
  console.log(i++);
  c();
}
noop = function() {}

// This will crash after a while
// on my computer: ~4300 calls
async.whilst(test, iter, noop);

The solution is simple, I've just added async.nextTick, similar to this pull request.

I've also taken the liberty to DRY up whilst and until. until now creates an anonymous function that negates the test it's given, then pass it along to async.whilst.

[caolan]

Your iterator is not asynchronous, that is why you're getting a stack overflow. This function is designed for asynchronous operations, while adding nextTick would stop it breaking on synchronous iterators, it's a good indicator that you're using the wrong tool and you should probably be using a normal while loop.

[Radagaisus]

Let's say our iterator does something like this:

var i, user_ids = [1,2,3]; // ...
iterator = function(cb) {
  // Get from cache, sync function
  user = cache.get_user(user_ids[i]);
  if (user) {
    i++;
    cb();
  } else {
   // Get from the DB, async
    mongo.get_user(user_ids[i++], cb);
  }
}

This might break. I think async should support this behavior of sometimes doing a sync and sometimes async work.

[sokra]

@Radagaisus 's code is reallistic and would break in some cases (many users, all cached).

I made a pull request (#135) which make whilst/until support sync and async cases.

[spollack]

I agree this is an issue worth fixing -- i just hit the same issue in my code (cached path is synchronous, non-cached path is asynchronous). That said, this issue affects more than just whilst/until in the async library -- other control flow options like forEachLimit are also affected as far as i can tell. So would be ideal to have a async-wide fix here. For now, i'm working around it manually by calling nextTick() on the otherwise synchronous paths...

[rymohr]

@caolan said "while adding nextTick would stop it breaking on synchronous iterators, it's a good indicator that you're using the wrong tool and you should probably be using a normal while loop"

Any tools you can suggest? Having to fallback to while loops, setTimeout calls, and duty calculations is a pain in the ass. Definitely not something you want to have to think about every time you need to work with a long running client-side operation.

[dougwilson]

You may want to check out Node.js's own documentation on nextTick which highly suggests you don't write functions that are sometimes sync and other times async, as it can cause unexpected behavior when the caller expects the function to be async (like this module).

[Radagaisus]

@dougwilson from the docs:

function definitelyAsync(arg, cb) {
  if (arg) {
    process.nextTick(cb);
    return;
  }

  fs.stat('file', cb);
}

Which is exactly the same as this pull request, we use nextTick even for sync operations.

[dougwilson]

No, the nextTick call needs to go in your iter function passed to whilst, not within whilst itself. whilst is designed to only work on async functions, and since your iter function is sync instead of async, that is why you get the stack overflow.

[seriousManual]

No, the nextTick call needs to go in your iter function passed to whilst, not within whilst itself.

+1

[caolan]

This kind of thing comes up fairly often so it's clearly confusing for some people. However, I consider this to be a useful indicator that your function is not following Node.js convention. If a function is asynchronous, it should always be asynchronous.

[chesles]

As long it's made clear what whilst and until expect, and that being inconsistently sync/async can be problematic (which isn't immediately obvious or intuitive), I agree with @dougwilson; the nextTick call, if needed, should be in the iterator, not whilst.

[seriousManual]

This is a very common problem that inflicts with many other subjects as well:
https://gist.github.com/1834492

If someAsynchrounosFunction would only sometimes behave asynchrounos, everything fails.
So if the interface of a function includes a callback it has always to be asynchrounos...

[sokra]

Why restricting the user to only async functions, or even advise him to use plattform specific code (process.nextTick)?

The problem can be solved in the async libary without nextTick and would not force the user to use a specific type of function. If he want mixed sync/async function he should be able to do so. (like in @Radagaisus 's example)

[seriousManual]

you don't have to use platform specific code as you can use async.nextTick.

this is a library for handling asynchrounos code, so why use it for synchrounos functions? :)

[sokra]

nextTick is slow compared to a while loop in pure js. :-|
To "forget" it in mixed function cause stack overflows in "rar" cases only which results in long debugging sessions... :(
setTimeout(fn, 0) (used in async.nextTick) is bad style and more a dirty hack. :-|

@Radagaisus 's example with mixed sync/async code and there are many more usecases. :)

[caolan]

Fixed in 6ad64ac