Load/unload native plugins on a per image basis #982
Conversation
|
Does this work on e.g. linux? |
|
I never tried on linux to be honest, but it worked like a charm on Mac during hackweek. I would have expected that behavior only if there is still a handle to the old library. So to my understanding only the situations like this are an issue: If we actually want do proper reloading in Unity we will need to copy the dll to Tmp/GUID/ (or similar) anyways since Windows keeps all loaded dlls locked so the user won't be able to replace it. In any case that would also work around the potential dlopen issue. Personally, I think the fix would be valuable even without considering this case, since without it we don't have any proper lifecycle/control for native plugins at all. |
|
Yeah, I think that the temp-copy workaround makes sense |
| } | ||
|
|
||
| static void | ||
| remove_cached_module(gpointer key, gpointer value, gpointer user_data) | ||
| remove_embedded_module (gpointer key, gpointer value, gpointer user_data) | ||
| { | ||
| mono_dl_close((MonoDl*)value); |
There was a problem hiding this comment.
Since you call g_strdup on key (name) above should you free it here?
| static void | ||
| remove_cached_module(gpointer key, gpointer value, gpointer user_data) | ||
| { | ||
| mono_dl_close((MonoDl*)value); |
There was a problem hiding this comment.
Same here are below. Should you g_free the key/name?
There was a problem hiding this comment.
I'd say most definitely! Interesting that mono didn't do that with the previous hash table. I'll fix it and retest the code.
Do you think running the full mono test suite should be enough to verify such a change? (Alternative would mean that I bring back the hackweek unity reloading hacks (nothing trunkworthy there yet) and manually check against that...)
…table-android Fix double gchandle free in thread detach logic (1062208)
…rict-mode Add option for strict write barriers
…debugger-sync Apply debugger changes from il2cpp 0c52b0b5b2babc2cbec886b8ef8015f682…
…offset-initialization Fix issue where get offset could return 0 when the type is uninited
…rict-mode Incremental boehm strict mode
Make gc params work for Boehm
Use LF line endings on all perl build scripts
…er types are encountered with 'unmanaged' constraint.
…image. Fixes issue mono#10201.
…9236 Properly inflate pointer types in inflate_generic_type (case 1069236)
…5895 Don't try to access metadata for dynamic method (case 1065895)
…-hang-on-connection-close-during-readwrite [tls] Fix read/write ignoring error code and requesting more read/write calls
…ert during client authentication
The thread local storage of sequence points and method execution contexts between IL2CPP and the mono debugger code was only being synchronized at certain times, mainly when breakpoints were processed. This could lead to a loss of synchronization after functions are exited and debugger frame commands accessing invalid stack data. This change adds synchronization for these data structures right before any managed method exit, when the method execution context for that method is destroyed. Also optimizing memory allocations by only allocating when the stack grows and just reusing the memory otherwise.
…xceptions Fix issue where loopback interface causes exception 1027045
…-socket-race-condition Windows support to abort blocking system calls. (mono#12654)
These files are now in the IL2CPP repo, so remove them here to avoid confusion.
Reverts part of fix for case 1073634
…3274 Use wrapped method if available (case 1093274)
…-agent Sync the debugger-agent.c with the IL2CPP repo
…3205 Grow StackSlotInfo array rather than asserting (case 1103205)
…roid-tickcount [Android] Fix the issue that Environment.TickCount returns wrong value…
…ocation [Case 1084800] Fix issue where TLS requests would reallocate a buffer when it could reuse it
Revert part of #1131 as it causes hangs in calls to CancelSynchronousIo
…r case 1070667
…ializer-fix Fix serialization issue with DataContractJsonSerializer UseSimpleDictionaryFormat (case 1070667)
…dfile-blocking Fix interrupting blocking file IO on Windows.
…etthreadcontext Bump bdwgc submodule to get fix for GetThreadContext (case 1114668)
…le-grow Update bdwgc
Upgrade automake to 1.16.1
Change stevedore repo to public
…aded one dlopen per module and image trust OS to do ref counting.
AndreasReich
force-pushed
the
unity-master-native-plugin-unload
branch
from
9b76ca5
to
8dfa276
Compare
|
Josh Peterson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Today, mono unloads native plugins only on shutdown: Every pinvoke will resolve the necessary dll/so exactly once and if not there put it into a global list which is only cleaned up when we do a complete mono shutdown.
This is problematic for Unity since it bars us from ever reloading plugins. With this PR, all dynamically loaded native plugins are kept in a per assembly-image list. This means of course also, that a native plugin may be "loaded" more than once by different assemblies, thus relying on the OS reference counting to not actually load it more than once.
Due to preloading of embedded libraries in mini/main.c, we keep the global list still around and query that one before looking at the per-image list (this part is new in comparison to the otherwise identical change in the hackweek project last week).
Note that in unity we do a independent dlopen on every native plugin and also never unload it again since this code path is commented out right now. So landing this change alone will have no direct effect on Unity. (which allows us to treat this change completely isolated)
Why do we want this:
Why might we not want to do this after all:
(todo there: uncomment unloading mechanism, look into plugin lifecycle callbacks (broken?!), do copy before load on windows to prevent file locking)
I'm obviously very much in favor of getting this in, but we need to discuss if this is too dangerous/too hard to maintain