Native loading: simplify git_libgit2_init / git_libgit2_shutdown
#1438
Conversation
LibGit2Sharp/Core/NativeMethods.cs
Outdated
| /// </summary> | ||
| private sealed class LibraryLifetimeObject : CriticalFinalizerObject | ||
| private static readonly NativeShutdownObject shutdownObject; | ||
|
|
There was a problem hiding this comment.
You no longer restore this warning. Was that intentional?
There was a problem hiding this comment.
It was not - thank you for catching this.
ethomson
force-pushed
the
ethomson/native_load
branch
from
ff3062b to
1764878
Compare
|
BTW this creates a merge conflict with #1318 so once you merge I'll do the best I can to resolve the conflict and update that PR, but please consider reviewing the merge commit to make sure I kept what you are trying to do here. |
|
Yeah - I feel like everything's going to create a merge conflict with #1318. 😛 I'm perfectly happy to wait on this until #1318 lands and deal with the merge conflicts myself - and I feel like we're very, very close to seeing #1318 land. Since you were the motivation behind this PR, I'm going to say that it's really up to you. |
|
I'm willing to go either way, but I guess it's easier for me if you hold off on merging this till #1318 is done. Thanks. |
|
Will do - thanks @AArnott ! |
ethomson
force-pushed
the
ethomson/native_load
branch
from
1764878 to
e6ef3da
Compare
Remove the refcounting of libgit2 usage leftover from when `SafeHandleBase` ensured that all handles were finished before calling `git_libgit2_shutdown`. libgit2 itself does this refcounting now, we no longer need to. We only need to call `git_libgit2_init` before the first call to libgit2 and `git_libgit2_shutdown` at the end.
Only set up the object with the `git_libgit2_shutdown` finalizer when `git_libgit2_init` has succeeded. This ensures that setting up the finalizer is the last thing that we do in the static constructor for `NativeMethods`, meaning that any exception trying to p/invoke `git_libgit2_init` remains catch-able.
.NET Core no longer supports CriticalFinalizerObject. We now get the strictest finalization guarantees simply inheriting Object. See https://github.com/dotnet/corefx/issues/1345
ethomson
force-pushed
the
ethomson/native_load
branch
from
e6ef3da to
b21e2f9
Compare
Move the
git_libgit2_initcall out of the object that contains the finalizer that does the correspondinggit_libgit2_shutdown. We do not want to create the object with that finalizer until we are sure that libgit2 has been loaded.Otherwise, that finalizer will be called even when the constructor threw an exception (probably from
git_libgit2_initnot finding the native library). In this case, when the finalizer is invoked, it will rethrow theTypeInitializationException, and exceptions from finalizers are not catchable.By ensuring that the object with the finalizer is only created after
git_libgit2_initsucceeds, then we can ensure that any failures ingit_libgit2_initremain catchable.Now that our object's lifecycle ensures that we must have called
git_libgit2_initin order for it to have been created, we can remove the unnecessary reference counting logic. We previously refcounted the native methods, with eachSafeHandleBaseupdating that reference count, so that we did not callgit_libgit2_shutdownbefore we were finished with the safe handle. But this is obsolete, and this refcounting was unused outside of the load / unload scenario. So we can simply do the unload in the finalizer, without trying to do any unnecessary work on the refcount.