Wrap git_trace_set to hold a reference to trace method to prevent GC. #915
Conversation
| /// </summary> | ||
| public static int git_trace_set(LogLevel level, git_trace_cb trace_cb) | ||
| { | ||
| static_trace_cb = trace_cb; |
There was a problem hiding this comment.
Why not GC.KeepAlive in the caller?
There was a problem hiding this comment.
I suppose, but that means all callers of git_trace_set() would need to do that to be completely safe. There's also the issue of when/if those callers could/should undo the KeepAlive on the method.
To me, it seemed that since this routine is stuffing a handle/object into LibGit2 for later use, it is responsible for holding the reference. And that reference should persist until we either clear or pass down a different object. (I guess technically, LibGit2 should call back into the CLR to declare that it is holding the reference, but this seemed more straight-forward.)
There was a problem hiding this comment.
KeepAlive is just a an empty non-inlined method that will keep the object alive. What 'undo' are you talking about? :P
If this should persist for more time, I'd rather have this documented and tell the user. I don't want this to be kept alive for lifetime if the user doesn't want it to. /cc @nulltoken
Having it kept inside LibGit2Sharp would mean never having it freed. That can impact the user in case we're using delegates which keep other things in memory. Rather leave it in user control rather than a cache that might fail way harder than we think it should?
While it's true that the user can set the value to null and free it, I don't know how sure I am of this way of doing it.
There was a problem hiding this comment.
For GC.KeepAlive check this and see how it works. It just makes an empty call that will trick the GC into thinking that the variable is used, so it can't reclaim it.
|
Thanks @jeffhostetler for this! I am not sure if using GC.KeepAlive would help here - I think the problem is that there is no managed reference to the delegate past the scope of the I wonder if explicitly storing the delegate for Assuming the above proposal even works, I think either the current solution proposed by @jeffhostetler (but I would move the static variable logic into the |
Have NativeMethod layer hold a reference to the trace handler that we give to LibGit2 so that it won't be GC'd if the caller doesn't persist it properly.
I do question why this was necessary since GlobalSettings is holding a static LogConfiguration which contains a reference to the handler.
I don't have a test for the change since I couldn't figure out how to cause logging to happen from a test since everything is private and libgit2 doesn't have many calls to logging.
Please advise.
Thanks.