Use Nan::MakeCallback to avoid random hangs

Calling v8::Function::Call directly for async callbacks leads to very
odd behavior in some circumstances, specifically the next tick of the
event loop seems to get stuck. After much digging, I found references to
this being a "known" issue, and the proper way to call async functions
in JS is to use the node::MakeCallback helper, or the Nan equivalent. In
some cases, we also use the Nan::Callback object as a helper for
managing JS callbacks.

KNOWN ISSUES: On node <v6, it seems that exceptions thrown from JS when
called from .NET do NOT propagate back to .NET correctly. Instead, they
terminate the JS process. This appears to be a known limitation of using
the MakeCallback method, with no easy workarounds. On 6.4.0, this test
does pass, which indicates the issue has been fixed in node.

src/common/v8synchronizationcontext.cpp

@@ -1,17 +1,17 @@
 /**
- * Portions Copyright (c) Microsoft Corporation. All rights reserved. 
- * 
+ * Portions Copyright (c) Microsoft Corporation. All rights reserved.
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- *  http://www.apache.org/licenses/LICENSE-2.0  
+ *  http://www.apache.org/licenses/LICENSE-2.0
  *
  * THIS CODE IS PROVIDED *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
- * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION 
- * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR 
- * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT. 
+ * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION
+ * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR
+ * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT.
  *
- * See the Apache Version 2.0 License for specific language governing 
+ * See the Apache Version 2.0 License for specific language governing
  * permissions and limitations under the License.
  */
 #include "edge_common.h"
@@ -33,7 +33,7 @@ unsigned long V8SynchronizationContext::v8ThreadId;
 uv_sem_t* V8SynchronizationContext::funcWaitHandle;
 uv_edge_async_t* V8SynchronizationContext::uv_edge_async;
 
-void V8SynchronizationContext::Initialize() 
+void V8SynchronizationContext::Initialize()
 {
     // This executes on V8 thread
 
@@ -61,19 +61,19 @@ uv_edge_async_t* V8SynchronizationContext::RegisterAction(uv_async_edge_cb actio
     {
         // This executes on V8 thread.
         // Allocate new uv_edge_async.
-
+        DBG("V8SynchronizationContext::RegisterAction on v8 thread");
         uv_edge_async_t* uv_edge_async = new uv_edge_async_t;
         uv_edge_async->action = action;
         uv_edge_async->data = data;
         uv_edge_async->singleton = FALSE;
         uv_async_init(uv_default_loop(), &uv_edge_async->uv_async, (uv_async_cb)continueOnV8Thread);
         return uv_edge_async;
     }
-    else 
+    else
     {
-        // This executes on CLR thread. 
+        // This executes on CLR thread.
         // Acquire exlusive access to uv_edge_async previously initialized on V8 thread.
-
+        DBG("V8SynchronizationContext::RegisterAction on CLR thread");
         uv_sem_wait(V8SynchronizationContext::funcWaitHandle);
         V8SynchronizationContext::uv_edge_async->action = action;
         V8SynchronizationContext::uv_edge_async->data = data;
@@ -84,7 +84,7 @@ uv_edge_async_t* V8SynchronizationContext::RegisterAction(uv_async_edge_cb actio
 void V8SynchronizationContext::ExecuteAction(uv_edge_async_t* uv_edge_async)
 {
     DBG("V8SynchronizationContext::ExecuteAction");
-    // Transfer control to completeOnV8hread method executing on V8 thread
+    // Transfer control to continueOnV8Thread method executing on V8 thread
     uv_async_send(&uv_edge_async->uv_async);
 }
 

src/dotnet/clrfunc.cpp

@@ -54,7 +54,7 @@ v8::Local<v8::Function> ClrFunc::Initialize(System::Func<System::Object^,Task<Sy
 
     v8::Local<v8::Value> factoryArgv[] = { Nan::New(proxyFunction), Nan::New<v8::External>((void*)wrap) };
     v8::Local<v8::Function> funcProxy =
-        (Nan::New(proxyFactory)->Call(Nan::GetCurrentContext()->Global(), 2, factoryArgv)).As<v8::Function>();
+        (Nan::MakeCallback(Nan::GetCurrentContext()->Global(), Nan::New(proxyFactory), 2, factoryArgv)).As<v8::Function>();
     Nan::Persistent<v8::Function> funcProxyPersistent(funcProxy);
     funcProxyPersistent.SetWeak((void*)wrap, &clrFuncProxyNearDeath, Nan::WeakCallbackType::kParameter);
 

src/dotnet/clrfuncinvokecontext.cpp

@@ -1,17 +1,17 @@
 /**
- * Portions Copyright (c) Microsoft Corporation. All rights reserved. 
- * 
+ * Portions Copyright (c) Microsoft Corporation. All rights reserved.
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- *  http://www.apache.org/licenses/LICENSE-2.0  
+ *  http://www.apache.org/licenses/LICENSE-2.0
  *
  * THIS CODE IS PROVIDED *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
- * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION 
- * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR 
- * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT. 
+ * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION
+ * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR
+ * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT.
  *
- * See the Apache Version 2.0 License for specific language governing 
+ * See the Apache Version 2.0 License for specific language governing
  * permissions and limitations under the License.
  */
 #include "edge.h"
@@ -21,12 +21,10 @@ ClrFuncInvokeContext::ClrFuncInvokeContext(v8::Local<v8::Value> callbackOrSync)
     DBG("ClrFuncInvokeContext::ClrFuncInvokeContext");
     if (callbackOrSync->IsFunction())
     {
-        this->callback = new Nan::Persistent<v8::Function>;
-        v8::Local<v8::Function> callbackOrSyncFunction = v8::Local<v8::Function>::Cast(callbackOrSync);
-        (this->callback)->Reset(callbackOrSyncFunction);
+        this->callback = new Nan::Callback(v8::Local<v8::Function>::Cast(callbackOrSync));
         this->Sync = false;
     }
-    else 
+    else
     {
         this->Sync = callbackOrSync->BooleanValue();
     }
@@ -41,7 +39,7 @@ void ClrFuncInvokeContext::DisposeCallback()
         DBG("ClrFuncInvokeContext::DisposeCallback");
         this->callback->Reset();
         delete this->callback;
-        this->callback = NULL;        
+        this->callback = NULL;
     }
 }
 
@@ -54,7 +52,7 @@ void ClrFuncInvokeContext::CompleteOnCLRThread(System::Threading::Tasks::Task<Sy
 
 void ClrFuncInvokeContext::InitializeAsyncOperation()
 {
-    // Create a uv_edge_async instance representing V8 async operation that will complete 
+    // Create a uv_edge_async instance representing V8 async operation that will complete
     // when the CLR function completes. The ClrActionContext is used to ensure the ClrFuncInvokeContext
     // remains GC-rooted while the CLR function executes.
 
@@ -119,17 +117,19 @@ v8::Local<v8::Value> ClrFuncInvokeContext::CompleteOnV8Thread()
     {
         // complete the asynchronous call to C# by invoking a callback in JavaScript
         Nan::TryCatch try_catch;
-        Nan::New<v8::Function>(*(this->callback))->Call(Nan::GetCurrentContext()->Global(), argc, argv);
+        DBG("ClrFuncInvokeContext::CompleteOnV8Thread - calling JS callback");
+        this->callback->Call(argc, argv);
         this->DisposeCallback();
-        if (try_catch.HasCaught()) 
+        if (try_catch.HasCaught())
         {
+            DBG("ClrFuncInvokeContext::CompleteOnV8Thread - exception in callback");
             Nan::FatalException(try_catch);
-        }        
+        }
 
         DBG("ClrFuncInvokeContext::CompleteOnV8Thread - async with callback");
         return scope.Escape(Nan::Undefined());
     }
-    else if (1 == argc) 
+    else if (1 == argc)
     {
         DBG("ClrFuncInvokeContext::CompleteOnV8Thread - handleScope.Close(ThrowException(argv[0]))");
         // complete the synchronous call to C# by re-throwing the resulting exception

src/dotnet/edge.h

@@ -1,17 +1,17 @@
 /**
- * Portions Copyright (c) Microsoft Corporation. All rights reserved. 
- * 
+ * Portions Copyright (c) Microsoft Corporation. All rights reserved.
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- *  http://www.apache.org/licenses/LICENSE-2.0  
+ *  http://www.apache.org/licenses/LICENSE-2.0
  *
  * THIS CODE IS PROVIDED *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
- * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION 
- * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR 
- * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT. 
+ * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION
+ * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR
+ * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT.
  *
- * See the Apache Version 2.0 License for specific language governing 
+ * See the Apache Version 2.0 License for specific language governing
  * permissions and limitations under the License.
  */
 #ifndef __EDGE_H
@@ -42,7 +42,7 @@ typedef struct clrActionContext {
 
 ref class ClrFuncInvokeContext {
 private:
-    Nan::Persistent<v8::Function>* callback;
+    Nan::Callback* callback;
     uv_edge_async_t* uv_edge_async;
 
     void DisposeCallback();
@@ -114,7 +114,7 @@ ref class NodejsFuncInvokeContext {
 ref class ClrFuncReflectionWrap {
 private:
     System::Object^ instance;
-    MethodInfo^ invokeMethod;    
+    MethodInfo^ invokeMethod;
 
     ClrFuncReflectionWrap();
 
@@ -138,7 +138,7 @@ ref class ClrFunc {
     v8::Local<v8::Value> Call(v8::Local<v8::Value> payload, v8::Local<v8::Value> callback);
     static v8::Local<v8::Value> MarshalCLRToV8(System::Object^ netdata);
     static v8::Local<v8::Value> MarshalCLRExceptionToV8(System::Exception^ exception);
-    static System::Object^ MarshalV8ToCLR(v8::Local<v8::Value> jsdata);    
+    static System::Object^ MarshalV8ToCLR(v8::Local<v8::Value> jsdata);
 };
 
 typedef struct clrFuncWrap {

src/dotnet/nodejsfuncinvokecontext.cpp

@@ -1,17 +1,17 @@
 /**
- * Portions Copyright (c) Microsoft Corporation. All rights reserved. 
- * 
+ * Portions Copyright (c) Microsoft Corporation. All rights reserved.
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
- *  http://www.apache.org/licenses/LICENSE-2.0  
+ *  http://www.apache.org/licenses/LICENSE-2.0
  *
  * THIS CODE IS PROVIDED *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
- * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION 
- * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR 
- * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT. 
+ * OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION
+ * ANY IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR
+ * PURPOSE, MERCHANTABLITY OR NON-INFRINGEMENT.
  *
- * See the Apache Version 2.0 License for specific language governing 
+ * See the Apache Version 2.0 License for specific language governing
  * permissions and limitations under the License.
  */
 #include "edge.h"
@@ -22,14 +22,14 @@ NAN_METHOD(v8FuncCallback)
     Nan::HandleScope scope;
     v8::Local<v8::External> correlator = v8::Local<v8::External>::Cast(info[2]);
     NodejsFuncInvokeContextWrap* wrap = (NodejsFuncInvokeContextWrap*)(correlator->Value());
-    NodejsFuncInvokeContext^ context = wrap->context;    
+    NodejsFuncInvokeContext^ context = wrap->context;
     wrap->context = nullptr;
     if (!info[0]->IsUndefined() && !info[0]->IsNull())
     {
         DBG("v8FuncCallback error");
         context->CompleteWithError(gcnew System::Exception(exceptionV82stringCLR(info[0])));
     }
-    else 
+    else
     {
         DBG("v8FuncCallback success");
         context->CompleteWithResult(info[1]);
@@ -70,34 +70,32 @@ void NodejsFuncInvokeContext::CallFuncOnV8Thread()
     static Nan::Persistent<v8::Function> callbackFunction;
 
     Nan::HandleScope scope;
-    try 
+    try
     {
         v8::Local<v8::Value> jspayload = ClrFunc::MarshalCLRToV8(this->payload);
 
         // See https://github.com/tjanczuk/edge/issues/125 for context
-        
+
         if (callbackFactory.IsEmpty())
         {
             v8::Local<v8::Function> v8FuncCallbackFunction = Nan::New<v8::FunctionTemplate>(v8FuncCallback)->GetFunction();
             callbackFunction.Reset(v8FuncCallbackFunction);
             v8::Local<v8::String> code = Nan::New<v8::String>(
                 "(function (cb, ctx) { return function (e, d) { return cb(e, d, ctx); }; })").ToLocalChecked();
             v8::Local<v8::Function> callbackFactoryFunction = v8::Local<v8::Function>::Cast(v8::Script::Compile(code)->Run());
-            callbackFactory.Reset(callbackFactoryFunction);            
+            callbackFactory.Reset(callbackFactoryFunction);
         }
 
         this->wrap = new NodejsFuncInvokeContextWrap;
         this->wrap->context = this;
         v8::Local<v8::Value> factoryArgv[] = { Nan::New(callbackFunction), Nan::New<v8::External>((void*)this->wrap) };
         v8::Local<v8::Function> callback = v8::Local<v8::Function>::Cast(
-            Nan::New(callbackFactory)->Call(Nan::GetCurrentContext()->Global(), 2, factoryArgv));        
+            Nan::MakeCallback(Nan::GetCurrentContext()->Global(), Nan::New(callbackFactory), 2, factoryArgv));
 
         v8::Local<v8::Value> argv[] = { jspayload, callback };
         Nan::TryCatch tryCatch;
-        DBG("NodejsFuncInvokeContext::CallFuncOnV8Thread calling JavaScript function");
-        Nan::New(*(this->functionContext->Func))->Call(Nan::GetCurrentContext()->Global(), 2, argv);
-        DBG("NodejsFuncInvokeContext::CallFuncOnV8Thread called JavaScript function");
-        if (tryCatch.HasCaught()) 
+        Nan::MakeCallback(Nan::GetCurrentContext()->Global(), Nan::New(*(this->functionContext->Func)), 2, argv);
+        if (tryCatch.HasCaught())
         {
             DBG("NodejsFuncInvokeContext::CallFuncOnV8Thread caught JavaScript exception");
             this->wrap->context = nullptr;
@@ -117,7 +115,7 @@ void NodejsFuncInvokeContext::Complete()
     {
         this->TaskCompletionSource->SetException(this->exception);
     }
-    else 
+    else
     {
         this->TaskCompletionSource->SetResult(this->result);
     }
@@ -133,7 +131,7 @@ void NodejsFuncInvokeContext::CompleteWithError(System::Exception^ exception)
 void NodejsFuncInvokeContext::CompleteWithResult(v8::Local<v8::Value> result)
 {
     DBG("NodejsFuncInvokeContext::CompleteWithResult");
-    try 
+    try
     {
         this->result = ClrFunc::MarshalV8ToCLR(result);
         Task::Run(gcnew System::Action(this, &NodejsFuncInvokeContext::Complete));

test/102_node2net.js

@@ -131,7 +131,7 @@ describe('async call from node.js to .net', function () {
             assert.equal(error.InnerException.ParamName, 'input');
             done();
         });
-    });    
+    });
 
     it('successfuly marshals empty buffer', function (done) {
         var func = edge.func({
@@ -176,4 +176,23 @@ describe('async call from node.js to .net', function () {
             done();
         })
     });
+
+    // Note: This doesn't seem to be sufficient to force the repro of the hang,
+    // but it's a good test to make sure works.
+    it('successfuly handles process.nextTick in the callback', function (done) {
+        var func = edge.func({
+            assemblyFile: edgeTestDll,
+            typeName: 'Edge.Tests.Startup',
+            methodName: 'ReturnInput'
+        });
+
+        var k = "";
+        func(k, function (error, result) {
+            assert.ifError(error);
+            assert.ok(result === k);
+            process.nextTick(function() {
+                done();
+            });
+        })
+    });
 });