Call git_threads_shutdown() after last handle get disposed #358

jbialobr · 2013-03-01T19:23:21Z

sharwell · 2013-03-01T19:46:22Z

If you have separate AddHandle and RemoveHandle methods, then you can use Interlocked.Increment and Interlocked.Decrement without a loop.

sharwell · 2013-03-01T19:50:29Z

Also, you should only call AddHandle(-1) (or RemoveHandle) from an implementation of SafeHandle.ReleaseHandle().

The runtime ensures this is only called once, where Dispose can (and is allowed to) be called any number of times.

sharwell · 2013-03-01T19:54:41Z

Your call to CheckForShutdown needs to check the value from the Interlocked method before the call, or git_threads_shutdown could be called more than once in multithreaded scenarios.

jbialobr · 2013-03-01T19:57:29Z

Thanks, it is my first time with Interlocked. Omitting bugy imple 8000 mentation, do you think it is an acceptable solution?

jamill · 2013-03-01T20:13:33Z

Do we care about the NotOwnedSafeHandle derived classes? There are a bunch of types that do not actually free resources (but we still want the type safety and consistency of deriving from SafeHandleBase).

nulltoken · 2013-03-01T20:15:06Z

Omitting bugy implementation, do you think it is an acceptable solution?

"The last one turns off the light" -> This sure looks like an interesting proposal.

/cc @xpaulbettsx @phkelley

sharwell · 2013-03-01T20:15:06Z

@jamill No, the finalization process for those handles never makes any calls into libgit2.

jamill · 2013-03-01T20:17:21Z

@sharwell correct - but with the current implementation here, they are still included in the handle counting (unless I am mistaken).

sharwell · 2013-03-01T20:19:50Z

The runtime will not call ReleaseHandle on these handles because they are constructed by calling the SafeHandle constructor that marks them as unowned handles. The handle count would never be decremented.

jbialobr · 2013-03-01T20:30:26Z

unless I am mistaken

NotOwnedSafeHandle inherits from SafeHandle

jamill · 2013-03-01T20:33:22Z

NotOwnedSafeHandle inherits from SafeHandle

Thanks - I missed that originally, I was mistaken - my apolgies.

sharwell · 2013-03-01T20:39:20Z

Since ReleaseHandle won't be called if the handle is invalid, we need to make sure that NativeMethods.AddHandle isn't called from the following constructors:

NullIndexSafeHandle
NullGitObjectSafeHandle

The concern at that point is what the P/Invoke layer does when a method returns the handle 0 - it will still construct the SafeHandle but it will be invalid so ReleaseHandle will never be called.

jbialobr · 2013-03-01T20:53:45Z

I would introduce new abstract function IsReleasable().

sharwell · 2013-03-01T20:55:55Z

It already exists as !(IsClosed||IsInvalid)

jbialobr · 2013-03-01T21:01:06Z

Yes but it can not be used to determine whether AddHandle or not. Maybe IsReleasable is not a good name. I meant that I would introduce a new function that will tell if AddHandle/RemoveHandle should be called.

sharwell · 2013-03-01T21:02:19Z

You can delay the call to AddHandle by placing it in SafeHandleBase.IsInvalid. When IsInvalid is returning false for the first time for an instance, call NativeMethods.AddHandle.

This is based on the following documentation: http://msdn.microsoft.com/en-us/library/system.runtime.interopservices.safehandle.releasehandle.aspx

The ReleaseHandle method is guaranteed to be called only once and only if the handle is valid as defined by the IsInvalid property.

jbialobr · 2013-03-01T21:08:04Z

Can you guarantee that invalid handle never becomes valid?

jbialobr · 2013-03-01T21:11:42Z

As I am not familiar with libgit handles: Can a valid handle becomes invalid?

jbialobr · 2013-03-01T21:13:57Z

You can delay the call to AddHandle by placing it in SafeHandleBase.IsInvalid. When IsInvalid is returning false for the first time for an instance, call NativeMethods.AddHandle.

I would prefer to call RemoveHandle from Dispose, ensuring it is called only once.

sharwell · 2013-03-01T21:16:57Z

As I am not familiar with libgit handles: Can a valid handle becomes invalid?

Yes, someone could call SafeHandle.SetHandleAsInvalid which would prevent the resource from ever being freed. The impact here would be failing to free resources (and finally failing to call git_threads_shutdown), but it wouldn't crash.

I would prefer to call RemoveHandle from Dispose, ensuring it is called only once.

SafeHandle.ReleaseHandle is guaranteed to be called only once. SafeHandle.Dispose can be freely called any number of times and the absolute expectation is this should never ever be a problem.

jbialobr · 2013-03-01T21:21:11Z

SafeHandle.ReleaseHandle is guaranteed to be called only once. SafeHandle.Dispose can be freely called any number of times and the absolute expectation is this should never ever be a problem.

Yes, but I can check if this is the first call to Dispose and only in that case call RemoveHandle

…dle will eventually be called)

sharwell · 2013-03-01T21:30:26Z

I sent you a pull request to cover the AddHandle case.

jbialobr · 2013-03-02T20:09:36Z

As we can not assume that handle does not become valid after ~LibraryLifetimeObject was called I consider IsInvalid approach as unsafe. I pushed other proposal which unregisters handle from both finalizer and dispose.

sharwell · 2013-03-02T20:13:17Z

My modified approach (the pull request yesterday) handles the case you refer to.

Call AddHandle from the constructor
Call RemoveHandle from IsInvalid if we determine that ReleaseHandle won't be called
Call RemoveHandle from ReleaseHandle for valid handles

jbialobr · 2013-03-02T20:17:36Z

What will happen when I call IsInvalid before it becomes valid?

sharwell · 2013-03-02T20:18:47Z

That would require calling IsInvalid from the constructor.

Edit: Or a derived type manipulating the value of protected field SafeHandle.handle in a non-standard manner (i.e. not recommended). Even then the fallback path for registration in IsInvalid would likely catch it when the calling code checked to make sure the handle was valid.

jbialobr · 2013-03-02T20:29:44Z

new LibraryLifetimeObject() //hendlesCount = 1
handle = new XXHanlde(); //hendlesCount = 2
hanlde.IsInvalid; //returns false //hendlesCount = 1
//handle becomes valid
//close app
~LibraryLifetimeObject(); // handle count becomes 0 and git_threads_shutdown() is called
handle.Dispose is called.

sharwell · 2013-03-02T20:31:29Z

//handle becomes valid

How is this going to happen?

jbialobr · 2013-03-02T20:36:02Z

Newly created SafeHandleBase is invalid. Then it becomes valid when its handle value becomes non zero.

sharwell · 2013-03-02T20:39:14Z

handle is a protected field that's assigned inside the P/Invoke layer before the handle is returned to the calling code. The only way to reassign it is through reflection or by deriving a new class from SafeHandleBase for the sole purpose of manipulating the shutdown process.

jbialobr · 2013-03-02T20:42:33Z

But IsInvalid is public and can be called at any time. If you add an assert that invalid handle never becomes valid after IsInvalid was called then I accept your solution.

sharwell · 2013-03-02T21:00:22Z

I'm about to send you a pull request with a "special" assertion that's safe for execution at this point in the application shutdown process, plus better documentation and a couple small tweaks.

…property (to prevent side effects)

sharwell · 2013-03-02T21:11:53Z

It automatically updated the previous pull request (now there are a total of 4 commits in it)

jbialobr · 2013-03-03T19:33:45Z

@sharwell I think your solution is 99.9% safe. It fails only when one uses handle in unusual way. My solution uses the same technique as LibraryLifetimeObject and it is resistant to cases that your solution is not. As you know this project and handles better, let me know if you see any weak points in my solution.

sharwell · 2013-03-03T21:30:08Z

What exactly do you define as "failure"? Currently I don't see any cases where yours handles a situation that mine does not.

Your code still calls Debug.Assert from RemoveHandle, which shouldn't be called from the finalizer.
My code detects and reports [most] situations where intentional misuse may result in the shutdown order breaking, and the report is made through an MDA which is reliable even during the CLR shutdown process.

sharwell · 2013-03-03T21:33:56Z

Your use of a finalizer is a big problem as well. See C# - What does “destructors are not inherited” actually mean?.

In your implementation, if a SafeHandleBase is not explicitly disposed then UnregisterHandle will be called before ReleaseHandle (which was the original problem).

jbialobr · 2013-03-04T10:07:30Z

Thanks, I removed finalizer - it is unneccessary (and badly used as you noticed) since ReleaseHandle is called from SafeHandle.Dispose. If SafeHandle.Dispose will not be called then ReleaseHandle also will not be called, thus I think it is enough to unregister hanlde from SafeHandleBase.Dispose.

What exactly do you define as "failure"? Currently I don't see any cases where yours handles a situation that mine does not.

Since IsInvalid is a part of public API, there should be no limitation to use it. It is very hard to define what
intentional misuse means at this level.

sharwell · 2013-03-04T14:29:51Z

There is no requirement that the CLR implement critical finalization of SafeHandle by calling Dispose. Now you are relying on an implementation detail to perform cleanup.

I believe the limitations on the use of IsInvalid in my implementation are completely reasonable. A user would have to derive a new type from SafeHandleBase and evaluate IsInvalid inside the constructor of that object, knowing full well that the handle is never valid at that point because the P/Invoke layer assigns the value after the object is constructed. External code can use IsInvalid anywhere/whenever.

This reverts commit 093fca9.

This reverts commit 3715ec2.

jbialobr · 2013-03-04T19:39:46Z

I believe the limitations on the use of IsInvalid in my implementation are completely reasonable. A user would have to derive a new type from SafeHandleBase and evaluate IsInvalid inside the constructor of that object, knowing full well that the handle is never valid at that point because the P/Invoke layer assigns the value after the object is constructed. External code can use IsInvalid anywhere/whenever.

I see that regular case is to obtain a handle from native method. I thought that handle can be set by external code but this would be very unusual to expose handle to public scope. Thanks, I learnt a lot.

sharwell · 2013-03-04T19:46:03Z

At this point I think we have a pretty solid solution worked out. Thanks for working through this with me 👍

nulltoken · 2013-03-11T22:23:45Z

Guys, this is an awesome collaboration!

Could you please consider squashing those commits altogether?

nulltoken · 2013-03-12T08:53:47Z

LibGit2Sharp/Core/NativeMethods.cs

+        {
+            int count = Interlocked.Decrement(ref handlesCount);
+            if (count == 0)
+                git_threads_shutdown();


Nitpick: Could you please surround this statement with braces?

jbialobr · 2013-03-12T13:04:32Z

@sharwell As the most of the code comes from you, could you please squash it to one commit and create a new pull request?

Call git_threads_shutdown() after last handle get disposed

a309c19

jbialobr mentioned this pull request Mar 1, 2013

Make git_threads_shutdown run after git finalizers #249

Merged

Corrected due to review comments

79d4def

Reverted changes done to SafeHandleBase.Dispose

9b864cd

jbialobr added 2 commits March 1, 2013 21:24

Decrement when removing

bdd45ed

Seal ReleaseHandle

cd70dbc

InternalReleaseHandle -> ReleaseHandleImpl

cbf5c69

Only call AddHandle when we know the handle is valid (i.e. ReleaseHan…

f90a967

…dle will eventually be called)

sharwell added 3 commits March 2, 2013 15:09

Make sure the debugger doesn't evaluate the SafeHandleBase.IsInvalid …

858f084

…property (to prevent side effects)

Safe and clearly documented global cleanup in SafeHandleBase

ab6c594

Update code for execution in a contrained execution region

ccef7be

Add ReliabilityContract attributes.

093fca9

jbialobr added 3 commits March 4, 2013 20:27

Revert "Add ReliabilityContract attributes."

5ca8226

This reverts commit 093fca9.

Revert "Unregister handle from finalizer either Dispose"

fb02cac

This reverts commit 3715ec2.

Merge branch 'pr/n2_sharwell' into vNext

3c02129

nulltoken reviewed Mar 12, 2013
View reviewed changes

jbialobr closed this Mar 12, 2013

sharwell mentioned this pull request Mar 12, 2013

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

Merged

Call git_threads_shutdown() after last handle get disposed #358

Call git_threads_shutdown() after last handle get disposed #358

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!