kriskowal / q

A promise library for JavaScript
MIT License
14.94k stars 1.21k forks source link

Q uses process.nextTick instead of setImmediate in Node 0.10.x #541

Open joe-spanning opened 10 years ago

joe-spanning commented 10 years ago

I am using Q 1.0.1 and Node.js 0.10.24

I have a application that uses Q to recursively process large amounts of data using the following pattern:

doPromiseyThing()
  .then(function(result) {
    if (moreToDo) {
      return doPromiseyThing();
    }

    return result;
  });

If there is a large amount of data to process in this way, I eventually get the error:

Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.

node.js:375
        throw new Error(msg);
              ^
Error: (node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
    at maxTickWarn (node.js:375:15)
    at process.nextTick (node.js:480:9)
    at onwrite (_stream_writable.js:260:15)
    at WritableState.onwrite (_stream_writable.js:97:5)
    at Socket._write (net.js:651:5)
    at doWrite (_stream_writable.js:221:10)
    at writeOrBuffer (_stream_writable.js:211:5)
    at Socket.Writable.write (_stream_writable.js:180:11)
    at Socket.write (net.js:613:40)
    at Console.warn (console.js:61:16)

This error is not caught by my fail handler function, so I cannot recover from it. It comes across as an uncaught exception in Node. However, this is not the point of me creating the ticket.

The error states that setImmediate should be used instead of process.nextTick. Looking at the source of q.js lines 158 - 177:

if (typeof process !== "undefined" && process.nextTick) {
        // Node.js before 0.9. Note that some fake-Node environments, like the
        // Mocha test runner, introduce a `process` global without a `nextTick`.
        isNodeJS = true;

        requestTick = function () {
            process.nextTick(flush);
        };

    } else if (typeof setImmediate === "function") {
        // In IE10, Node.js 0.9+, or https://github.com/NobleJS/setImmediate
        if (typeof window !== "undefined") {
            requestTick = setImmediate.bind(window, flush);
        } else {
            requestTick = function () {
                setImmediate(flush);
            };
        }

    }

It prefers process.nextTick to setImmediate.

If would be nice if this code was changed to use setImmediate, or if I could configure Q to prefer setImmediate over process.nextTick via an environment variable.

dsimmons commented 10 years ago

+1

I too just started having the same exact problem. I'm also processing large amounts of data, using recursive promises, to elegantly handle both rate limiting and calls for subsequent pages where I don't know where the end is.

It hums along great for several hours (was watching htop the entire time to monitor for potential memory leaks), but eventually terminates with the above error.

dantman commented 10 years ago

to elegantly handle both rate limiting and calls for subsequent pages where I don't know where the end is.

On this note I should note a separate relevant, IE has flawed handling of its setImmediate function. In some cases setTimeout loops and DOM modifications can result in setImmediate callbacks never being called. http://codeforhire.com/2013/09/21/setimmediate-and-messagechannel-broken-on-internet-explorer-10/

dsimmons commented 10 years ago

@dantman: that's a valid concern that should be taken into consideration, but just in the interest of debugging this: at least for my case, it's entirely in a server-side environment with no front-end interaction.

jimlloyd commented 9 years ago

@joe-spanning: Thanks for logging this issue, you saved me some debugging time. I am using the exact same pattern. My code looks like:

function onScriptInput() {
  self.promptForInput()
    .then(lineReader.readLine)
    .then(self.onEchoInput)
    .then(self.compileAndExecuteScript)
    .then(self.printResult)
    .catch(function(err) { console.error(chalk.bold.red(err)); })
    .done(onScriptInput);
}

It seems I can work around the problem by inserting a delay of 1ms:

function onScriptInput() {
  self.promptForInput()
    .then(lineReader.readLine)
    .then(self.onEchoInput)
    .then(self.compileAndExecuteScript)
    .then(self.printResult)
    .catch(function(err) { console.error(chalk.bold.red(err)); })
    .delay(1)
    .done(onScriptInput);
}
sideroad commented 9 years ago

+1

riegelTech commented 6 years ago

+1, this is a bad design that produces heavy effects when application load increases, late in development flow :-(