-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cross library interleaving can cause node 10's nextTick limit to be hit. #337
Conversation
nextTick actually isn't deprecated and node 11 has removed this limit. Although setImmediate may not be correct here, we may need to use it in node 10.. |
cc @domenic |
So the issue you see is multiple different RSVP's interacting with each other. This could also be caused by multiple different promises libraries interacting with each other in specific ways. var A = require('rsvp1').Promise;
var B = require('rsvp2').Promise;
A.resolve().then(function() {
console.log('first nextTick');
B.resolve().then(function() {
console.log('second nextTick');
A.resolve().then(function() {
console.log('third nextTick');
});
});
}); @taras ^^ if looped 500 or so times will cause the error to happen, likely a great test case to add. this problem can happen quickly in for example broccoli were depending on the topology of node_modules, we can easily have a large number of rsvp's floating around |
I wonder actually if joliss/promise-map-series#4 may mitigate this problem. I'll have to investigate the exact scenario in broccoli in broccoli that builds this specific problem |
via @domenic in node 11: |
i suspect everyone else also has this problem, even though asap guards. It can't possibly guard when interleaving with other libraries occurs. bluebird: when: asap: |
@taras i have a test scenario you can implement. |
@stefanpenner got it, I'll work on that. |
@taras thank you sir :) |
btw the sky is falling :P |
@@ -24,7 +24,7 @@ var isWorker = typeof Uint8ClampedArray !== 'undefined' && | |||
// node | |||
function useNextTick() { | |||
return function() { | |||
process.nextTick(flush); | |||
setImmediate(flush); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets do this only if node is < 0.11.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deprecation was introduced in 0.10.0. Should I make this change for 0.10.x?
It took me too long to figure out that "the sky is falling" means that it's time sensitive :) I'm working on it now. |
I'm having a hard time replicating the error with your code example. Can you give me a gist with code that you used to cause the deprecation to show up? |
@stefanpenner I pushed a bit of WIP code to show you what I've been trying. Note: I'm manually copying the |
var A = require('rsvp1').Promise;
A.resolve().then(function() {
console.log('first nextTick');
process.nextTick(function() { // <-- should be sufficient i think
A.resolve().then(function() {
console.log('third nextTick');
});
});
}); |
Here is the test that I tried, but I wasn't able to get it show the deprecation warning.
|
ya that inner tree needs to have a depth of 1000, not repeating the whole thing 1000 times. |
Got it. The following code causes warnings to be displayed.
When I apply this code to async.js, the warnings go away.
Now, I gotta figure out how to capture output. |
I don't need to capture output to get deprecation errors because that test will cause If that's ok, then you can look at the code. |
Grr... it looks like the test doesn't run 500 times with setImmediate. I'm going to look into it further. Sorry. |
- Added test that causes Maximum call stack size exeeded error in node Added test that causes recursion
@stefanpenner ok, I got it sorted out. I verified that my new test code creates a recursion 1000 levels deep. With original code, the test fails because an exception is thrown. The tests passes on the new code. I also added node 0.11.x to travis. |
@taras awesome this looks fantastic. I really appreciate you digging into this! |
Replaced deprecated process.nextTick with setImmediate
It's my pleasure. Glad I could help out. |
Great tag team work, big ups for getting to the bottom of this. |
On Node 0.10.33, using RSVP causes deprecation error:
(node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
This issue seems to appear and disappear under strange circumstances:
This PR replaces process.nextTick with setImmediate. All tests pass.