Make git_threads_shutdown run after git finalizers

niik · niik · commit 247ca1a30cfb · 2012-11-22T10:15:29.000+01:00
When initiating a repository in order to extract a diff between the last commit and the working directory and 'forget'/ignore to properly dispose of the Repository instance you'll get an AccessViolationException on application shutdown. The problem is that the finalizer for Repository attempts to call git_repository_free after git_threads_shutdown has been called meaning that TLS-state which is potentially required as part of a shutdown isn't available. git_threads_shutdown is supposed to be called right before "shutting down" the library. See [global.c L16-40][1]. At the moment the call to git_threads_shutdown is done from the [AppDomain.ProcessExit][2] event. The problem is that the MSDN doesn't guarantee that [AppDomain.ProcessExit][2] runs after finalizers (on my machine it never does and I'm guessing that's actually the expected behavior). Here's an example of how to reproduce it: ```c# static void Main(string[] args) { // path to repo which contains changes in the working directory. var repo = new Repository(@"e:\code\misc\libgit2sharp\"); var changes = repo.Diff.Compare(); } ``` This will throw an AccessViolationException in git_repository_free. The obvious and correct way to fix this is to simply make sure that the repo instance gets disposed. ```c# static void Main(string[] args) { // path to repo which contains changes in the working directory. using(var repo = new Repository(@"e:\code\misc\libgit2sharp\")) var changes = repo.Diff.Compare(); } ``` One way to tackle this problem would be to clearly state that you really have to dispose your repositories but since libgit2sharp seems to be doing a lot of work to shut down cleanly in finalizers today I thought we should at least discuss it. My hackish workaround is to create an object which derives from [CriticalFinalizerObject][3] which calls git_threads_init in the constructor and git_threads_shutdown in the destructor. Since this object is a CriticalFinalizerObject its destructor will run after any 'regular' destructors such as the Repository destructor thus ensuring that git_threads_shutdown is the last thing to happen on the PInvoke-side before the AppDomain is completely done. From the blog of [Chris Brumme][4]: > All normal finalizable objects are either executed or discarded without > finalization, before any critical finalizers are executed. [1]: https://github.com/libgit2/libgit2/blob/development/src/global.c#L16-L40 [2]: http://msdn.microsoft.com/en-us/library/system.appdomain.processexit.aspx [3]: http://msdn.microsoft.com/en-us/library/system.runtime.constrainedexecution.criticalfinalizerobject.aspx [4]: http://blogs.msdn.com/b/cbrumme/archive/2004/02/20/77460.aspx
diff --git a/LibGit2Sharp/Core/NativeMethods.cs b/LibGit2Sharp/Core/NativeMethods.cs
@@ -3,6 +3,7 @@
 using System.IO;
 using System.Reflection;
 using System.Runtime.CompilerServices;
+using System.Runtime.ConstrainedExecution;
 using System.Runtime.InteropServices;
 using LibGit2Sharp.Core.Handles;
 
@@ -13,6 +14,21 @@ internal static class NativeMethods
     {
         public const uint GIT_PATH_MAX = 4096;
         private const string libgit2 = "git2";
+        private static readonly LibraryLifetimeObject lifetimeObject;
+
+        /// <summary>
+        /// Internal hack to ensure that the call to git_threads_shutdown is called after all handle finalizers 
+        /// have run to completion ensuring that no dangling git-related finalizer runs after git_threads_shutdown. 
+        /// There should never be more than one instance of this object per AppDomain.
+        /// </summary>
+        private sealed class LibraryLifetimeObject : CriticalFinalizerObject
+        {
+            // Ensure mono can JIT the .cctor and adjust the PATH before trying to load the native library. 
+            // See https://github.com/libgit2/libgit2sharp/pull/190
+            [MethodImpl(MethodImplOptions.NoInlining)]
+            public LibraryLifetimeObject() { NativeMethods.git_threads_init(); }
+            ~LibraryLifetimeObject() { NativeMethods.git_threads_shutdown(); }
+        }
 
         static NativeMethods()
         {
@@ -29,21 +45,8 @@ static NativeMethods()
                                                    String.Format(CultureInfo.InvariantCulture, "{0}{1}{2}", path, Path.PathSeparator, Environment.GetEnvironmentVariable(pathEnvVariable)));
             }
 
-            GitInit();
-            AppDomain.CurrentDomain.ProcessExit += ThreadsShutdown;
-        }
-
-        [MethodImpl(MethodImplOptions.NoInlining)]
-        private static void GitInit()
-        {
-            // keep this in a separate method so mono can JIT the .cctor
-            // and adjust the PATH before trying to load the native library
-            git_threads_init();
-        }
-
-        private static void ThreadsShutdown(object sender, EventArgs e)
-        {
-            git_threads_shutdown();
+            // See LibraryLifetimeObject description.
+            lifetimeObject = new LibraryLifetimeObject();
         }
 
         public static string ProcessorArchitecture
