Skip to content

Guaranteed relative finalization order for SafeHandleBase and git_threads_shutdown() #361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 12, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/ConfigurationSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class ConfigurationSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_config_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/DiffListSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ namespace LibGit2Sharp.Core.Handles
{
internal class DiffListSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_diff_list_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/GitObjectSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class GitObjectSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_object_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/IndexSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class IndexSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_index_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/NoteSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class NoteSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_note_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/NullGitObjectSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public NullGitObjectSafeHandle()
handle = IntPtr.Zero;
}

protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
// Nothing to release
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/NullIndexSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public NullIndexSafeHandle()
handle = IntPtr.Zero;
}

protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
// Nothing to release
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/ObjectDatabaseSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class ObjectDatabaseSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_odb_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/PushSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class PushSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_push_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/ReferenceSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class ReferenceSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_reference_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/RemoteSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class RemoteSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_remote_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/RepositorySafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class RepositorySafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_repository_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/RevWalkerSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class RevWalkerSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_revwalk_free(handle);
return true;
Expand Down
77 changes: 74 additions & 3 deletions LibGit2Sharp/Core/Handles/SafeHandleBase.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System;
using System.Diagnostics;
using System.Globalization;
using System.Runtime.ConstrainedExecution;
using System.Runtime.InteropServices;
using Interlocked = System.Threading.Interlocked;

namespace LibGit2Sharp.Core.Handles
{
Expand All @@ -11,9 +13,18 @@ internal abstract class SafeHandleBase : SafeHandle
private readonly string trace;
#endif

/// <summary>
/// This is set to non-zero when <see cref="NativeMethods.AddHandle"/> has
/// been called for this handle, but <see cref="NativeMethods.RemoveHandle"/>
/// has not yet been called.
/// </summary>
private int registered;

protected SafeHandleBase()
: base(IntPtr.Zero, true)
{
NativeMethods.AddHandle();
registered = 1;
#if LEAKS
trace = new StackTrace(2, true).ToString();
#endif
Expand All @@ -35,11 +46,71 @@ protected override void Dispose(bool disposing)
}
#endif

public override bool IsInvalid
// Prevent the debugger from evaluating this property because it has side effects
[DebuggerBrowsable(DebuggerBrowsableState.Never)]
public override sealed bool IsInvalid
{
get
{
bool invalid = IsInvalidImpl();
if (invalid && Interlocked.CompareExchange(ref registered, 0, 1) == 1)
{
/* Unregister the handle because we know ReleaseHandle won't be called
* to do it for us.
*/
NativeMethods.RemoveHandle();
}

return invalid;
}
}

[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
protected virtual bool IsInvalidImpl()
{
get { return (handle == IntPtr.Zero); }
return handle == IntPtr.Zero;
}

protected abstract override bool ReleaseHandle();
[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
protected abstract bool ReleaseHandleImpl();

protected override sealed bool ReleaseHandle()
{
bool result;

try
{
result = ReleaseHandleImpl();
}
finally
{
if (Interlocked.CompareExchange(ref registered, 0, 1) == 1)
{
// if the handle is still registered at this point, we definitely
// want to unregister it
NativeMethods.RemoveHandle();
}
else
{
/* For this to be called, the following sequence of events must occur:
*
* 1. The handle is created
* 2. The IsInvalid property is evaluated, and the result is false
* 3. The IsInvalid property is evaluated by the runtime to determine if
* finalization is necessary, and the result is now true
*
* This can only happen if the value of `handle` is manipulated in an unexpected
* way (through the Reflection API or by a specially-crafted derived type that
* does not currently exist). The only safe course of action at this point in
* the shutdown process is returning false, which will trigger the
* releaseHandleFailed MDA but have no other impact on the CLR state.
* http://msdn.microsoft.com/en-us/library/85eak4a0.aspx
*/
result = false;
}
}

return result;
}
}
}
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/SignatureSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class SignatureSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_signature_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/TreeBuilderSafeHandle.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ namespace LibGit2Sharp.Core.Handles
{
internal class TreeBuilderSafeHandle : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_treebuilder_free(handle);
return true;
Expand Down
2 changes: 1 addition & 1 deletion LibGit2Sharp/Core/Handles/TreeEntrySafeHandle_Owned.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{
internal class TreeEntrySafeHandle_Owned : SafeHandleBase
{
protected override bool ReleaseHandle()
protected override bool ReleaseHandleImpl()
{
Proxy.git_tree_entry_free(handle);
return true;
Expand Down
31 changes: 29 additions & 2 deletions LibGit2Sharp/Core/NativeMethods.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
using System;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.ConstrainedExecution;
using System.Runtime.InteropServices;
using System.Threading;
using LibGit2Sharp.Core.Handles;

// ReSharper disable InconsistentNaming
Expand All @@ -15,6 +17,7 @@ internal static class NativeMethods
public const uint GIT_PATH_MAX = 4096;
private const string libgit2 = "git2";
private static readonly LibraryLifetimeObject lifetimeObject;
private static int handlesCount = 0;

/// <summary>
/// Internal hack to ensure that the call to git_threads_shutdown is called after all handle finalizers
Expand All @@ -26,8 +29,32 @@ 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() { Ensure.ZeroResult(git_threads_init()); }
~LibraryLifetimeObject() { git_threads_shutdown(); }
public LibraryLifetimeObject()
{
Ensure.ZeroResult(git_threads_init());
AddHandle();
}

~LibraryLifetimeObject()
{
RemoveHandle();
}
}

[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
internal static void AddHandle()
{
Interlocked.Increment(ref handlesCount);
}

[ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
internal static void RemoveHandle()
{
int count = Interlocked.Decrement(ref handlesCount);
if (count == 0)
{
git_threads_shutdown();
}
}

static NativeMethods()
Expand Down