Make git_threads_shutdown run after git finalizers #249
Conversation
|
I should add that I'm not overly excited about this workaround. It feels a wee bit dirty but hopefully it's a starting point. |
|
Nice patch. Man I wish we didn't have to do this. I wonder if @nulltoken has any clever ideas :) |
|
There's been some interest in adding a non-global context to libgit2. This would mean that there are no more calls to |
|
@dahlbyk It would make it very obvious that it shouldn't be instantiated by anyone else. I wasn't sure about the coding conventions when I wrote it. I'll fix. @ethomson I agree that would be the optimal solution. Any gut feeling as to when we might see progress in that area? As I stated earlier this feels like a very low priority issue since it's kind of self-inflicted when you don't dipose. On the other hand, from reading the code, the library amibition seems to be nice cleanup even without dispose and the AVException is a bit intimidating. Looking around at some SO-questions and such it's not uncommon to see code which doesn't dispose properly. Especially for people writing one-off "script-ish" console applications. |
|
@markus-olsson Looks like a very well thought PR! I just wonder how well that would play on Mono considering 3627335 /cc @joncham |
|
@nulltoken yep, I saw that commit. It didn't occur to me at the time that constructor calls would ever get inlined but a quick glance at the mono_method_check_inlining in the mono jitter (btw; how awesome is it that I can do that!) suggests there's no opt-outs for normal constructors, only classes with static constructors. I'm guessing there's two ways of solving this, either we just decorate the constructor with a MethodImpl attribute to prevent any inlining or we keep the call to git_threads_init in a private static method of the NativeMethods class and just use the lifetime object for finalizing. My knee jerk reaction would be to decorate the constructor call. It's not like it's gonna hurt if the method isn't inlined. I'm adding the attribute now and then I'll revert it if we decide otherwise |
|
@markus-olsson Looks fine. Last nitpick: could you please squash the commits into a nice cohesive one and remove the code beautification changes? |
|
@nulltoken I had a hunch you'd want me to :) Sure. And I'll re-include the comment about why to prevent inlining in the first place since I seem to have missed that. Darn it, the hotkey for formatting a document is muscle memory by now but it wasn't meant to go in the repo. I'll fix. |
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
|
@nulltoken there you go. |
|
@markus-olsson Awesome troubleshooting and very clean fix! ✨ Merged in vNext |
|
I still get AV on exit from GitExtensions buiild from commit d7aecc0. Steps to reproduce:
|
|
It looks like this happens when
AFAIK even if the finalizer of I can think of two options and I sincerely hope there are others because none of them really appeal to me:
Thoughts? |
|
I agree with Paul's statement. You should take a very close look at the behavior of Last I checked, the |
|
Why you not implement finalizer for |
|
Finalizers in C#/.NET don't work the way (most) people expect. If you don't have a rigorously justified reason for creating one for a very specific purpose where |
|
Ok, but at least finalizer for |
|
Omitting the call to
|
|
So in conclusion no side effects at all from removing the call completely? In that case I'm all for it! 👍 |
|
For normal usage, definitely not a problem to remove it. For a specific case where someone is creating multiple new |
|
Yes. AppDomains! I knew that there was something on my mind when I investigated this for the CFO hack. I wouldn't have thought it was even possible to load native libraries multiple times from a process so I guess we don't need to worry about that. Is the nop-behavior of git_threads_init documented or is it an implementation detail? |
|
It's an implementation detail, but probably wouldn't affect a decision on this issue anyway |
|
But you don't know what else git_threads_shutdown will be doing in the future. |
|
|
We could request that libgit2 document |
Could it be called from AppDomain.ProcessExit? |
|
or |
|
I think removing the call and getting official documentation that the call to |
I don't like this idea at all. I would never accept such a request. |
Problem
If you initiate 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("Attempted to read or write protected memory. This is often an indication that other memory is corrupt.") on application shutdown.The problem is that
Repositorydestructor attempts to callgit_repository_freeaftergit_threads_shutdownhas been called which I'm guessing means that thread local state which is potentially required as part of a shutdown isn't available.git_threads_shutdownis supposed to be called right before "shutting down" the library. See global.c L16-40. At the moment the call togit_threads_shutdownis done from the AppDomain.ProcessExit event.The problem is that the MSDN doesn't guarantee that AppDomain.ProcessExit runs after finalizers (on my machine it doesn't and I'm guessing that's actually the expected behavior).
Reproduce it
Here's an example of how to reproduce it:
This will throw an
AccessViolationExceptioningit_repository_freewhen the GC calls theRepositorydestructor. The obvious and correct way to fix this is to simply make sure that the repo instance gets disposed first:Possible solution
One way to tackle this problem would be to ignore it and just clearly state the importance of properly disposing 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 which calls git_threads_init in the constructor and git_threads_shutdown in the destructor. Since this object is a
CriticalFinalizerObjectits destructor will run after any 'regular' destructors such as theRepositorydestructor thus ensuring thatgit_threads_shutdownis the last thing to happen on the PInvoke-side before the AppDomain is completely done.From the blog of Chris Brumme: