Comments (27)
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.
from edge.
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.
from edge.
I understand the arguments to use exports. I was looking on it from a pure C# assembly perspective.
from edge.
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.
from edge.
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
from edge.
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 thatNodejsFunc::FunctionWrapper
uses.
It may actually be not as bad as it sounds. Thoughts?
from edge.
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?
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
I will continue this investigation.
from edge.
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.
from edge.
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).
from edge.
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?
from edge.
Good catch, see analysis in #22.
from edge.
I also pushed [email protected] to npm with the latest binaries and the fix for #22.
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
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.
from edge.
Great. Are you in a position to do a pull request yet?
from edge.
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.
from edge.
This is now fixed. Thank you Dirk!
from edge.
Related Issues (20)
- Cannot find module 'C:\native\win32\x64\12.13.0\edge_nativeclr' HOT 1
- Is it possible to return something different than an async Task<object> HOT 2
- ReferenceError: __dirname is not defined HOT 1
- 什么时候支持到 nodejs 14 以上的版本呢? HOT 1
- Cant compile x86 DLL on node.js
- can i building Edge.js NuGet package with .net 5.0? HOT 2
- Crashing when calling Process.GetCurrentProcess().MainModule.FileName() HOT 1
- aww
- MS Build Error: Specify a project or solution file. The current working directory does not contain a project or solution file. HOT 1
- Edge-js not getting installed on Azure VM Ubuntu 20.4 HOT 1
- Getting TargetParameterCountException when calling a C# DLL HOT 1
- Edge.js library is not recognized by VS Code Poject. HOT 1
- Node JS v18 support HOT 3
- Edge using a dll failed if the dll is compiled as 32bits
- Will Edge.js NuGet package works with .net 6.0 HOT 1
- Get eventlog no permission error when referencing a dll file
- what time it can support vite vue3 HOT 1
- Added into awesome directory HOT 4
- Activating extension 'undefined_publisher.newextension' failed: The specified procedure could not be found. \\?\s:\Work\VSExtension\node_modules\edge-js-up\lib\native\win32\x64\18.14.0\edge_nativeclr.node. HOT 2
- Error in using Electron Forge packaging in Electron application. HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from edge.