Skip to content
This repository
Browse code

Use reference counting in SafeHandleBase to ensure git_threads_shutdo…

…wn() only gets called after the last handle is released (joint work with Sam Harwell)
  • Loading branch information...
commit fbcef4418c1fa80d0329bd9ff31f63a4dd3a4956 1 parent e1205a2
Janusz Białobrzewski jbialobr authored sharwell committed
2  LibGit2Sharp/Core/Handles/ConfigurationSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class ConfigurationSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_config_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/DiffListSafeHandle.cs
@@ -2,7 +2,7 @@ namespace LibGit2Sharp.Core.Handles
2 2 {
3 3 internal class DiffListSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_diff_list_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/GitObjectSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class GitObjectSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_object_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/IndexSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class IndexSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_index_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/NoteSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class NoteSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_note_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/NullGitObjectSafeHandle.cs
@@ -9,7 +9,7 @@ public NullGitObjectSafeHandle()
9 9 handle = IntPtr.Zero;
10 10 }
11 11
12   - protected override bool ReleaseHandle()
  12 + protected override bool ReleaseHandleImpl()
13 13 {
14 14 // Nothing to release
15 15 return true;
2  LibGit2Sharp/Core/Handles/NullIndexSafeHandle.cs
@@ -9,7 +9,7 @@ public NullIndexSafeHandle()
9 9 handle = IntPtr.Zero;
10 10 }
11 11
12   - protected override bool ReleaseHandle()
  12 + protected override bool ReleaseHandleImpl()
13 13 {
14 14 // Nothing to release
15 15 return true;
2  LibGit2Sharp/Core/Handles/ObjectDatabaseSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class ObjectDatabaseSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_odb_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/PushSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class PushSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_push_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/ReferenceSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class ReferenceSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_reference_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/RemoteSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class RemoteSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_remote_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/RepositorySafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class RepositorySafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_repository_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/RevWalkerSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class RevWalkerSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_revwalk_free(handle);
8 8 return true;
77 LibGit2Sharp/Core/Handles/SafeHandleBase.cs
... ... @@ -1,7 +1,9 @@
1 1 using System;
2 2 using System.Diagnostics;
3 3 using System.Globalization;
  4 +using System.Runtime.ConstrainedExecution;
4 5 using System.Runtime.InteropServices;
  6 +using Interlocked = System.Threading.Interlocked;
5 7
6 8 namespace LibGit2Sharp.Core.Handles
7 9 {
@@ -11,9 +13,18 @@ internal abstract class SafeHandleBase : SafeHandle
11 13 private readonly string trace;
12 14 #endif
13 15
  16 + /// <summary>
  17 + /// This is set to non-zero when <see cref="NativeMethods.AddHandle"/> has
  18 + /// been called for this handle, but <see cref="NativeMethods.RemoveHandle"/>
  19 + /// has not yet been called.
  20 + /// </summary>
  21 + private int registered;
  22 +
14 23 protected SafeHandleBase()
15 24 : base(IntPtr.Zero, true)
16 25 {
  26 + NativeMethods.AddHandle();
  27 + registered = 1;
17 28 #if LEAKS
18 29 trace = new StackTrace(2, true).ToString();
19 30 #endif
@@ -35,11 +46,71 @@ protected override void Dispose(bool disposing)
35 46 }
36 47 #endif
37 48
38   - public override bool IsInvalid
  49 + // Prevent the debugger from evaluating this property because it has side effects
  50 + [DebuggerBrowsable(DebuggerBrowsableState.Never)]
  51 + public override sealed bool IsInvalid
  52 + {
  53 + get
  54 + {
  55 + bool invalid = IsInvalidImpl();
  56 + if (invalid && Interlocked.CompareExchange(ref registered, 0, 1) == 1)
  57 + {
  58 + /* Unregister the handle because we know ReleaseHandle won't be called
  59 + * to do it for us.
  60 + */
  61 + NativeMethods.RemoveHandle();
  62 + }
  63 +
  64 + return invalid;
  65 + }
  66 + }
  67 +
  68 + [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
  69 + protected virtual bool IsInvalidImpl()
39 70 {
40   - get { return (handle == IntPtr.Zero); }
  71 + return handle == IntPtr.Zero;
41 72 }
42 73
43   - protected abstract override bool ReleaseHandle();
  74 + [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
  75 + protected abstract bool ReleaseHandleImpl();
  76 +
  77 + protected override sealed bool ReleaseHandle()
  78 + {
  79 + bool result;
  80 +
  81 + try
  82 + {
  83 + result = ReleaseHandleImpl();
  84 + }
  85 + finally
  86 + {
  87 + if (Interlocked.CompareExchange(ref registered, 0, 1) == 1)
  88 + {
  89 + // if the handle is still registered at this point, we definitely
  90 + // want to unregister it
  91 + NativeMethods.RemoveHandle();
  92 + }
  93 + else
  94 + {
  95 + /* For this to be called, the following sequence of events must occur:
  96 + *
  97 + * 1. The handle is created
  98 + * 2. The IsInvalid property is evaluated, and the result is false
  99 + * 3. The IsInvalid property is evaluated by the runtime to determine if
  100 + * finalization is necessary, and the result is now true
  101 + *
  102 + * This can only happen if the value of `handle` is manipulated in an unexpected
  103 + * way (through the Reflection API or by a specially-crafted derived type that
  104 + * does not currently exist). The only safe course of action at this point in
  105 + * the shutdown process is returning false, which will trigger the
  106 + * releaseHandleFailed MDA but have no other impact on the CLR state.
  107 + * http://msdn.microsoft.com/en-us/library/85eak4a0.aspx
  108 + */
  109 + result = false;
  110 + }
  111 + }
  112 +
  113 + return result;
  114 + }
44 115 }
45 116 }
2  LibGit2Sharp/Core/Handles/SignatureSafeHandle.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class SignatureSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_signature_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/TreeBuilderSafeHandle.cs
@@ -2,7 +2,7 @@ namespace LibGit2Sharp.Core.Handles
2 2 {
3 3 internal class TreeBuilderSafeHandle : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_treebuilder_free(handle);
8 8 return true;
2  LibGit2Sharp/Core/Handles/TreeEntrySafeHandle_Owned.cs
@@ -2,7 +2,7 @@
2 2 {
3 3 internal class TreeEntrySafeHandle_Owned : SafeHandleBase
4 4 {
5   - protected override bool ReleaseHandle()
  5 + protected override bool ReleaseHandleImpl()
6 6 {
7 7 Proxy.git_tree_entry_free(handle);
8 8 return true;
31 LibGit2Sharp/Core/NativeMethods.cs
... ... @@ -1,10 +1,12 @@
1 1 using System;
  2 +using System.Diagnostics;
2 3 using System.Globalization;
3 4 using System.IO;
4 5 using System.Reflection;
5 6 using System.Runtime.CompilerServices;
6 7 using System.Runtime.ConstrainedExecution;
7 8 using System.Runtime.InteropServices;
  9 +using System.Threading;
8 10 using LibGit2Sharp.Core.Handles;
9 11
10 12 // ReSharper disable InconsistentNaming
@@ -15,6 +17,7 @@ internal static class NativeMethods
15 17 public const uint GIT_PATH_MAX = 4096;
16 18 private const string libgit2 = "git2";
17 19 private static readonly LibraryLifetimeObject lifetimeObject;
  20 + private static int handlesCount = 0;
18 21
19 22 /// <summary>
20 23 /// Internal hack to ensure that the call to git_threads_shutdown is called after all handle finalizers
@@ -26,8 +29,32 @@ private sealed class LibraryLifetimeObject : CriticalFinalizerObject
26 29 // Ensure mono can JIT the .cctor and adjust the PATH before trying to load the native library.
27 30 // See https://github.com/libgit2/libgit2sharp/pull/190
28 31 [MethodImpl(MethodImplOptions.NoInlining)]
29   - public LibraryLifetimeObject() { Ensure.ZeroResult(git_threads_init()); }
30   - ~LibraryLifetimeObject() { git_threads_shutdown(); }
  32 + public LibraryLifetimeObject()
  33 + {
  34 + Ensure.ZeroResult(git_threads_init());
  35 + AddHandle();
  36 + }
  37 +
  38 + ~LibraryLifetimeObject()
  39 + {
  40 + RemoveHandle();
  41 + }
  42 + }
  43 +
  44 + [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
  45 + internal static void AddHandle()
  46 + {
  47 + Interlocked.Increment(ref handlesCount);
  48 + }
  49 +
  50 + [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
  51 + internal static void RemoveHandle()
  52 + {
  53 + int count = Interlocked.Decrement(ref handlesCount);
  54 + if (count == 0)
  55 + {
  56 + git_threads_shutdown();
  57 + }
31 58 }
32 59
33 60 static NativeMethods()

0 comments on commit fbcef44

Please sign in to comment.
Something went wrong with that request. Please try again.