Skip to content
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

Issues executing C++ addon in the context of a Fiber. Native Promise and nextTick broken. #350

Closed
gschmottlach-xse opened this issue Aug 17, 2017 · 2 comments

Comments

@gschmottlach-xse
Copy link

I have created a separate (simple/small) GitHub project to illustrate the issue I'm seeing using Fibers with C++ addons. The project can be found here:
https://github.com/gschmottlach-xse/node-fiber-test

The project demonstrates a Node.js program that calls from JavaScript into a native C++ method which in turn calls back out to JavaScript to execute a function and return a result. This entire call chain in executed within the context of a single fiber.

What I'm seeing is that when the JavaScript method is called from the C++ addon both the native Node.js Promise.resolve() method and process.nextTick() cease to function. Specifically, Promise.resolve() will not chain to the next then() function in a Promise chain. Functions registered to be processed by Node's nextTick() function are never executed.

I did discover a work-around but it is less than satisfactory since I do not understand the underlying issue that is causing this behavior. Substituting the Bluebird Promise implementation and using setImmedate() instead of process.nextTick() seems to resolve the issue that I am seeing. Unfortunately, in larger projects using third-party modules I cannot guarantee this work-around can be deployed so I'm trying to understand if it's a limitation of Node.js Fibers, Node.js, or an error on my part.

There doesn't appear to be an issue if the C++ addon module is not involved in the call-chain. Unfortunately, for my application, I cannot eliminate that step through the native component. Fortunately, my sample project exposes a very simple C++ addon module (one method and the constructor).

I would very much appreciate it if you can spare some time, download and run my minimal application. I hope you can tell me whether I've encountered a known limitation of Fibers, Node.js, or it an a misunderstanding on my part. The project includes a README.md that provides additional details and instructions on how to run the simple test application to both cause the error and alternatively work-around it using BlueBird's Promise implementation and setImmedate() in place of process.nextTick().

I'm hoping you have a better understanding of Node's internal operation and it's interaction with the Fiber module.Thanks for any assistance you can provide. . . .

@laverdet
Copy link
Owner

Hey thanks for the thorough report, it made it much easier for me to reproduce and get to the bottom of this.

It looks like the issue here is the usage of Nan::Callback instead of Nan::Call. In this case you should be using Nan::Call. Nan::Callback should be used when you're invoking a JS callback from an external event, for instance a direct libuv callback. On the other hand if you're invoking a JS method synchronously Nan::Call is better. I think the functions are more forgiving when used without fibers.

If I apply the following diff to your example then it executes cleanly:

diff --git a/cpp/stack.cpp b/cpp/stack.cpp
index d46da0c..c50ba81 100644
--- a/cpp/stack.cpp
+++ b/cpp/stack.cpp
@@ -85,9 +85,10 @@ NAN_METHOD(Stack::ReadData)
 
     const int argc = 2;
     v8::Local<v8::Value> argv[argc] = {info[0], info[1]};
-    Nan::Callback callback(readDataFunc);
-    v8::Local<v8::Value> result = callback.Call(readDataFunc, argc, argv);
-    return info.GetReturnValue().Set(result);
+    Nan::MaybeLocal<v8::Value> result = Nan::Call(readDataFunc, v8::Local<v8::Object>::Cast(Nan::Undefined()), argc, argv);
+    if (!result.IsEmpty()) {
+        info.GetReturnValue().Set(result.ToLocalChecked());
+    }
 }

gschmottlach-xse pushed a commit to gschmottlach-xse/node-fiber-test that referenced this issue Aug 18, 2017
@gschmottlach-xse
Copy link
Author

Thank you so much for taking the time to help me resolve this issue. I'm far from a Node.js C++ addon expert but I vaguely remembered seeing a post about an issue with Nan:Callback but did not fully understand the implications described in that thread. I don't think I'd ever caught this problem in my addon code without your help. I re-worked my original addon code where I originally detected the problem. It now works as expected. I always had a gnawing feeling that I was missing something and feel much better now that I understand the problem although the subtleties between Nan::Callback and Nan::Call will require more study.

Thanks again . . .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants