Enable calling JS functions from C# after the Task has been completed about edge

[dbaeumer]

I like that idea a lot and it would solve my use case. However I would argue that the property should be named import since for me the JS code is 'imported' into the C# code.

[tjanczuk]

Well you would be authoring node.js code here, so from that perspective it is an export. Plus node.js folks are already used to the notion of exports from authoring node.js modules. I think exports is more appropriate.

[dbaeumer]

I understand the arguments to use exports. I was looking on it from a pure C# assembly perspective.

[dbaeumer]

I looked into how to fix this and here is my proposal:

• as suggest in the first comment we pass a exports literal when we bind the C# function
• if present we store a Dictionary with the CLR marshaled data with the ClrFunc created in ClrFunc::Initialize
• if present we pass that dictionary as an additional argument to the function call
• we could add an unbind function property to the function returned from edge.func to unbind the CLR function and to clean-up the Dictionary with the exports from the bind.

Let me know if you think that is the right way to implement that feature and I continue working on it.

[dbaeumer]

Instead of passing the exports to the function call we can set them into the CLR object if it has a visible imports property. Something like

public class Startup {
public IDictionary<string, object> imports;
}

I like this better than passing it as an arg to the function. It will avoid parameter miss matches

[tjanczuk]

The reason I originally liked the idea of specifying exports at the time of calling edge.func is that I hoped we could smuggle these functions naturally to C# at the time of the actual call without adding complexity either to the general interop model or the implementation. But that would require either modifying the payload the user supplied in the call, or - as you suggested - modifying the delegate from Func<object,Task<object>> to Func<object,object,Task<object>>. Neither of these feel very clean.

This is the point where I think I'd rather look into adding complexity to the implementation than the model itself. Let's explore the original concept and try to allow calling to the JavaScript functions from C# for as long as the Func<object,Task<object>> proxy around it is not garbage collected by CLR. This means allowing it to outlive the completion of the Task that originally received a proxy to the JavaScript function as part of its payload.

Off the top of my mind, here are the changes that would be required:

• stop releasing V8 persistent handles around the exported JavaScript functions when the Task associated with the original entry to C# completes,
• add logic to the finalizer of NodejsFunc that will release the V8 persistent handle. The complexity of this is related to thread synchronization. The finalizer will be called on a CLR thread, and the V8 persistent handle can only be released on the singleton V8 thread. This will require switching threads to V8 to complete the finalization, using similar mechanism that NodejsFunc::FunctionWrapper uses.

It may actually be not as bad as it sounds. Thoughts?

[dbaeumer]

I further investigated this and I was able to call the JS function even after the original CLR call has finished by doing:

    public async Task<object> Invoke(IDictionary<string, object> payload) {
        Func<object, Task<object>> changed = (Func<object, Task<object>>)payload["changed"];
        Task.Delay(200).ContinueWith(async (value) => {
            Console.WriteLine("Delay Task finished - Calling Callback");
            await changed(null);
            Console.WriteLine("Changed Callback from Delayed Task returned in CLR");
        });
        return "CLR function ended";
    }

But there is more to it than simply stopping releasing the V8 handles associated with the exported JavaScript function. These functions refer to the original CLR function invocation context which gets released when the original function returns. So the context can't be used anymore to switch from the CLR thread to the V8 thread to call the JS function. I see a couple of options.

We add finalizer logic to ClrFuncInvokeContext as well to clean up the uv_async structures and don't do it anymore when the original call finishes. However we would still need to switch from the finalizer in both ClrFuncInvokeContext and NodejsFunc to the V8 thread to release the handles for which we can't use the original ClrFuncInvokeContext since the CLR GC does't guaruntee any order in which finalizers are called. And even if it did we need to 'dispose' the ClrFuncInvokeContext in the V8 thread. Right?

We make the fact that we need to keep the context around more explicit by using 'reference counting and explicit disposing':

• in the contributed C# code we allow to 'mark' functions as fully persistent. That is the place where we know this the best anyways. However we can still do the exports or a variant of it to specify full persistent functions.
• if we have one fully persistent marked function we keep the invocation context in tact (e.g. don't call DisposeUvEdgeAsync(), DisposeUvEdgeAsyncFunc())
• we somehow provide a Dispose method back to JS which allows disposing the ClrFuncInvokeContext.

I am leaning towards the second one. In the use case of listening to events from C# someone in the JS land has to manage the listeners anyway. So for example we could dispose the ClrFuncInvokeContext when the last listener got removed.

Thoughts?

[dbaeumer]

Here is a snippet that shows how the second solution looks like:

public async Task<object> Invoke(IDictionary<string, object> payload, Func<Func<object, Task<object>>, bool> markForOutofScopeUse) {
    Func<object, Task<object>> changed = (Func<object, Task<object>>)payload["changed"];
    markForOutofScopeUse(changed);
    Task.Delay(200).ContinueWith(async (value) => {
        Console.WriteLine("Delay Task finished - Calling Callback");
        await changed(null);
        Console.WriteLine("Changed Callback from Delayed Task returned in CLR");
    });
    return "CLR function ended";
}

Please note that the second arg of the Invoke method is optional. If the method declaration doesn't have a second arg the Edge interop library doesn't create the markForOutOfScope function delegate. So for standard case the signature of the Invoke method still stays simple.

[tjanczuk]

I think there is an opportunity for deterministic cleanup without changes to the programming model.

I also think we should avoid extending the lifetime of the ClrFuncInvokeContext beyond the moment the task completed.

Here are a few ideas around it:

• We should factor out the code that switches back from CLR thread to V8 thread into its own component. It is already used in a few places now and I foresee it will be used in a few more places later. That will break the dependency of the NodejsFunc on the ClrFuncInvokeContext. This component can be static, since there is only one node.js thread in the process. The component will be responsible for creating the uv_async structures, maintaining the WaitHandle to synchronize multi-threaded access to the uv_async instance from CLR thread pool, and posting completions to libuv IOCP.
• Every instance of NodejsFunc we create should not be referenced by any CLR instance maintained by edge.js. The only thing keeping an instance of NodejsFunc from being garbage collected should be the fact it is in the closure of the Func<object,Task<object>> delegate we pass over to C# code. This means the instance will only be garbage collected when the C# code decides to drop the last reference to the Func delegate.
• In the finalizer of NodejsFunc we can use the shared component to switch back to V8 thread and release the persistent handle of the V8 function.

Does this make sense? I think I am going to take a stab at teasing out the shared synchronization component, as I need it to fix #11 which I need soon for some other prototype that builds on top of edge.js. But it would be great if you continued this investigation.

[dbaeumer]

Just to check if I understand it correctly: you want to share one uv_async instance to communicate from the CLR thread pool back to the V8 thread and that instance will basically live as long as the edge module is loaded. Makes perfect sense to me and will make things a lot easier. I tried to keep the association between the uv_async instance and the CLR function call.

[dbaeumer]

I will continue this investigation.

[tjanczuk]

Yes, that is the idea. I am not sure yet if we can use literally one instance, or whether we will need to re-create them on a per call basis, let me see how the code pans out. But you can assume there will me a mechanism for code on CLR thread to run a continuation on V8 thread.

[tjanczuk]

I checked in the refactored, centralized thread synchronization logic with 8891a4a and 537c615.

As part of the latter I believe the second bullet point above was also addressed.

What remains to be done on this bug is to refactor the current disposal logic for functions exported from node.js to .NET from an explicit model based on completion of the ClrFuncInvokeContext Task to an implicit model based on garbage collection of NodejsFunc. (The third bullet above).

[dbaeumer]

Thanks for providing the centralized thread synchronization. I caught up with it and started to work on basing the NodejsFunc on garbage collection however I deadlock immediately. Looking at it deeper I found a interesting behavior (the deadlock is independent of the garbage collection changes I did). My test case is as follows:

C# code:

public class Startup {
    public async Task<object> WithCallbackDeadLock(IDictionary<string, object> payload) {
        Func<object, Task<object>> changed = (Func<object, Task<object>>)payload["changed"];
        Console.WriteLine("Calling changed callback");
        await changed(null);
        return "CLR function ended";
    }
}

Node code:

function changedFunc(data, callback) {
    console.log('Changed called'); 
    callback(null, 'Sending back value');
}

var withCallbackDeadLock = edge.func({
    assemblyFile: 'native.dll',
    typeName: 'Testing.Startup',
    methodName: 'WithCallbackDeadLock',
});

withCallbackDeadLock({
    changed: changedFunc
}, function(error, result) {
    console.log(result);
})


withCallbackDeadLock({
    changed: changedFunc
}, function(error, result) {
    console.log(result);
})

The interesting thing is that in this case everything is executed in the V8 thread although the withCallbackDeadLock C# method is marked as async and contains an await. I changed the DBG message and added System::Threading::Thread::CurrentThread->ManagedThreadId to see on which managed thread the code is executed and here is the trace:

V8SynchronizationContext::Initialize - 1
V8SynchronizationContext::Unref - 1
ClrFunc::Initialize - 1
ClrFunc::Initialize - 1
ClrFunc::Call - 1
ClrFuncInvokeContext::ClrFuncInvokeContext - 1
V8SynchronizationContext::RegisterActionFromV8Thread - 1
NodejsFunc::NodejsFunc - 1
ClrFuncInvokeContext::AddPersistentHandle - 1
Calling changed callback
NodejsFunc::FunctionWrapper - 1
NodejsFuncInvokeContext::NodejsFuncInvokeContext - 1
V8SynchronizationContext::RegisterActionFromCLRThread - 1
V8SynchronizationContext::ExecuteAction - 1
ClrFunc::Call - 1
ClrFuncInvokeContext::ClrFuncInvokeContext - 1
V8SynchronizationContext::RegisterActionFromV8Thread - 1
NodejsFunc::NodejsFunc - 1
ClrFuncInvokeContext::AddPersistentHandle - 1
Calling changed callback
NodejsFunc::FunctionWrapper - 1
NodejsFuncInvokeContext::NodejsFuncInvokeContext - 1
V8SynchronizationContext::RegisterActionFromCLRThread - 1

The reason why it is dead locking is quite obvious: the first call to the CLR function executes its callback which calls funcWaitHandle->WaitOne() and PostQueuedCompletionStatus and then unwinds the stack. Since we get back to V8/node the next call to withCallbackDeadLock is executed which the waits on funcWaitHandle->WaitOne() in the V8/node thread.

Changing the C# code to

    public Task<object> WithCallback(IDictionary<string, object> payload) {
        Func<object, Task<object>> changed = (Func<object, Task<object>>)payload["changed"];
        Console.WriteLine("Calling changed callback");
        return Task.Run(async () => {
            await changed(null);
            return (object)"CLR function ended";
        });
    }

makes it work since it actually does the work in a separate thread.

I don't know enough about CLR to understand why the first one doesn't spawn a separate thread. Could be me as well doing something wrong. However if my example is ok then I see the following options to fix this:

• detect the case where the CLR function is executed in main and bypass the whole thread syncing
• use a different locking mechanism that is reentrant when hit in the same thread. I will start locking if I can find something
• force CLR to spawn a thread in this case. However I don't know how.

Any thoughts on this?

[tjanczuk]

Good catch, see analysis in #22.

[tjanczuk]

I also pushed [email protected] to npm with the latest binaries and the fix for #22.

[dbaeumer]

Thanks for fixing the deadlock issue. I continued working on adding a finalizer to NodejsFunc. Adding the finalizer was easy but it gets never called even if I force a GC. The same is true for the finalizer on NodejsFuncInvokeContext.

I attached a heap profiler to node and the profiler tells me that NodejsFunc instance is referenced by NodejsFuncInvokeContext which is referenced by a GCHandle. However Edge doesn't create these GCHandles and the only place I can think of where this happens in when edge adds the NodejsFuncInvokeContext as the target into a System::Action and pass it via uv_async back to the V8 thread. Since we transition in this situation from managed into unmanaged code CLR might do some magic for us. However this is pure speculation. I will continue to investigate deeper into this however any hints why the NodejsFuncInvokeContext is referenced by a GCHandle are welcome.

[dbaeumer]

Additional note: the way how I will try to fix it (and to validate above 'speculation') is to create the GCHandle in the CLR thread for the context before transition into the V8 thread and release the GCHandle when we are back in the CLR thread.

[tjanczuk]

I assume you have removed the call to AddPersistentHandle from the constructor of NodejsFunc?

I wonder of the problem is with the cyclic dependency that NodejsFuncInvokeContextWrap has with the NodejsFuncInvokeContext that owns it: https://github.com/tjanczuk/edge/blob/master/src/nodejsfuncinvokecontext.cpp#L28-29. Currently that cyclic dependency is only broken in the finalizer of NodejsFuncInvokeContex. It may be worth trying to break it by cleaning up the wrap as soon as control returns to the JavaScript's function callback in v8FuncCallback: https://github.com/tjanczuk/edge/blob/master/src/nodejsfuncinvokecontext.cpp#L10.

[tjanczuk]

The wrap should also be clean up if the JS function invocation results in an exception in https://github.com/tjanczuk/edge/blob/master/src/nodejsfuncinvokecontext.cpp#L68. In that case the callback might not get called.

[dbaeumer]

Thanks for the pointer. I search for GCHandle in edge. I wasn't aware of the gcroot template. Learned something! This explains why NodejsFuncInvokeContext is referenced by a GCHandle.

[dbaeumer]

This did the trick. I will clean up the code and will write test cases. One question: to write a test case I need to trigger a CLR gc. I would add a .testing property on the edge JS that exposes a function to trigger the CLR gc. Any objections to recommendation how to do this best?

I already implemented the native to trigger the GC to run my local tests.

[tjanczuk]

Sounds great.

Isn't there a way to trigger GC right from C# (http://msdn.microsoft.com/en-us/library/system.gc.aspx)? Perhaps you can just call from JS to C# to trigger garbage collection from within the test?

One more thought that occurred to me: it is probably prudent to move the NodejsFuncInvokeContextWrap instantiation from the constructor of NodejsFuncInvokeContex to right before it is actually needed in https://github.com/tjanczuk/edge/blob/master/src/nodejsfuncinvokecontext.cpp#L62 to reduce the risk of these circular references leading to memory leaks.

[dbaeumer]

Yes, that is what i use. but wasn't thinking about using edge to call the GC. Cool idea.

I will move the allocation of the NodejsFuncInvokeContextWrap as you suggests.

[tjanczuk]

Great. Are you in a position to do a pull request yet?

[dbaeumer]

One thing I noticed while implementing this features is that call into JS from C# when the task as ended are usually used as event callbacks. Such event callbacks into JS usually aren't await by C# code. So we could simplify at least the behaviour of the JS function.I will open a separate issue for this.

[tjanczuk]

This is now fixed. Thank you Dirk!