Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Compatibility With Nodejs 0.10.0 #267

Closed
Jud opened this Issue Mar 12, 2013 · 15 comments

Comments

Projects
None yet
4 participants

Jud commented Mar 12, 2013

With the process.nextTick change in 0.10.0, detectSeries fails to work.

(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.

util.js:35
  var str = String(f).replace(formatRegExp, function(x) {
                      ^
RangeError: Maximum call stack size exceeded
Contributor

brianmaissy commented Mar 12, 2013

What code are you using that results in that warning?

I'm not sure I understand the problem.

Here is the change made to nextTick in 0.10:

process.nextTick happens at the end of the current tick, immediately after the current stack unwinds. If you are currently using recursive nextTick calls, use setImmediate instead.

-- https://github.com/joyent/node/wiki/Api-changes-between-v0.8-and-v0.10

If a function foo invokes itself via nextTick and then returns, the next invocation of foo will happen while we're still on the same tick. So all the recursive calls to foo will be made after the stack unwinds but still in the same tick, and control will never be returned to the event loop.

// Bad, because will not yield to event loop
function foo(){
  longRunningSynchronousCode();
  if(predicate) nextTick(foo);
}

But assuming no long-running synchronous code exists in foo, I don't see why this is a problem. That is to say if we were actually trying to yield to IO with each call to nextTick, I would see why this recursive usage doesn't work. But since our purpose in using nextTick is ONLY to make sure that we return before executing the function, then continuing to use it recursively isn't a problem.

// Perfectly fine?
function foo(){
  asynchronousCode(function(){
    if(predicate) nextTick(foo);
    somethingElse(); // this still happens before foo is called again
  });
}

The fix is obviously simple, to replace nextTick with setImmediate if it's available, but I'm not sure it's necessary in this case.

Contributor

brianmaissy commented Mar 12, 2013

I'll also add this:

However, there are programs out in the wild that use recursive calls to process.nextTick to avoid pre-empting the I/O event loop for long-running jobs. In order to avoid breaking horribly right away, Node will now print a deprecation warning, and ask you to use setImmediate for these kinds of tasks instead.

-- http://blog.nodejs.org/2013/03/11/node-v0-10-0-stable/

It doesn't look like they're talking to us. Providing an asynchronous iterator will not result in this message.

That said, changing this might be part of "idiot-proofing" async against people using it with synchronous functions

Jud commented Mar 12, 2013

I have an array that looks like:

arr = [10, 5, 0, 0, 0, 0, 2, 2, 10, 0, 0, 0]

When given an arbitrary position (pos) and length (len) I want to return the len values "above" and "below" pos that are non-zero.

I have used detectSeries (which could be the wrong choice, but I needed a function that had a short-circuit). In my case arr has ~1000000 elements, if that matters.

Here is what the code for getting the first len elements above pos looks like.

    above = []
    async.detectSeries(arr[pos...arr.len], ((el, should_stop) ->
      if el is not 0
        above.push el
        if above.length >= len then should_stop yes else should_stop no
      else
        should_stop no
    ), cb)

I tried forking and changing to setImmediate, but that caused the final callback, cb to never be called.

This is the code that is causing those errors.

Jud commented Mar 12, 2013

Actually, I just tested it again, and it appears that detectSeries alone, with only should_stop no in the body will trigger this error.

Contributor

brianmaissy commented Mar 12, 2013

Yeah, there's nothing asynchronous about your iterator, that's why you're getting the warning.

I propose changing to setImmediate, @caolan what do you think?

Owner

caolan commented Mar 12, 2013

The more I think about it, the more I feel like we should leave all this stuff up to the user. "Idiot-proofing" the library against misuse is just kicking the can down the road IMHO. I'm thinking we should remove detection of sync tasks and automatic nextTick/setImmediate calls. Thoughts?

Jud commented Mar 12, 2013

Yeah, there's nothing asynchronous about your iterator, that's why you're getting the warning.

@brianmaissy, I'm a little confused by this. How would you propose that this code become "asynchronous" (if it already isn't)?

async.detectSeries(arr, ((entry, should_stop) ->
  should_stop no
), cb)

^ The above code breaks as well

Jud commented Mar 12, 2013

Here is my POC that demonstrates this:

async = require 'async'

arr = []

for x in [0...1001]
  arr.push x

async.detectSeries(arr, ((el, should_stop) ->
  should_stop no
), -> console.log 'done')

It breaks when arr.length is >1001

Contributor

brianmaissy commented Mar 12, 2013

@Jud:

It's not asynchronous because it calls back in the same tick of the event loop. Therefore all 1001 calls to the iterator happen without yielding to the event loop, which Node doesn't like. That's why they manually limited recursive nextTick calls to 1000 and throw a warning, because they foresaw this happening by mistake.

Wrapping your iterator in a call to setImmediate() will solve the problem. That way, the iterator will only be invoked once per event loop, and the warning will go away.

@caolan:

Setting aside the issue of idiot-proofing for a second:

In any case, it's valuable to stick to a convention of always returning from the async library functions before calling iterators or callbacks, whether or not they are invoked at the end of the tick with nextTick or on a future tick with setImmediate. Not as idiot-proofing, as sticking to a control flow convention.

Take this code for example:

var arr = ['file1','file2','file3'];
async.map(arr, fs.stat, function(err, results){
    console.log('a');
});
console.log('b');

It isn't clear what will be logged first, a or b. As it turns out, b will be logged first. But if the array was empty, a will be logged first. We've already seen a few instances of when this can be significant. The anonymous function passed as a callback to map isn't asynchronous. Does that make the author of the above code an "idiot" that we need to "proof" against? On the contrary, I think this is correct usage, and this is something that a good API should standardize. The node documentation says as much here: http://nodejs.org/api/process.html#process_process_nexttick_callback.

I think it would be in order to wrap all invocations of iterators or callbacks with a call to nextTick(), to control the order of invoking and returning.

Now to return to the issue of idiot-proofing:

However, many users of async also pass synchronous iterators from time to time, not just synchronous callbacks. And running a synchronous iterator over a large collection, even with nextTick, will choke the event loop. To avoid this, it is fair to ask the users to wrap synchronous iterators in a call to setImmediate. Doing it ourselves inside async will cause a noticeable slowdown in the case of a large array, otherwise I would err on the side of idiot-proofing here. But in any case, the fact that all iterators passed to async must be asynchronous if you want to yield to the event loop, should be clearly emphasized in the readme.

Jud commented Mar 12, 2013

@brianmaissy Ah! My confusion came from initially using eachSeries and after a cursory overview of the lib, I saw this line:

https://github.com/caolan/async/blob/master/lib/async.js#L135

and assumed that the library wrapped all iterators in async.nextTick.

Anyway, my 2c is that the library should be consistent one way or another. I'd be happy to send a pull request for documentation additions either way.

Owner

caolan commented Mar 13, 2013

@Jud agreed, we should definitely be consistent. I personally believe it should consistently be the users problem ;)

Jud commented Mar 14, 2013

@caolan that sounds like the best solution.

I'm encountering the same issue in v0.10.15 with async v0.2.5. Is this issue going to be fixed soon?

Tried async v0.2.10. The error is no more reported.

Owner

caolan commented Feb 5, 2014

@kadishmal you need to make sure your functions are running asynchronously - if you don't know whether it'll be asynchronous or not you can force it to be async using setImmediate

@caolan caolan closed this Feb 5, 2014

I've made appropriate changes to force async code. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment