Kill ThreadAffinity

[nulltoken]

See #854 (comment)

[carlosmn]

The issue here is that libgit2 stores the error message in a thread-local variable. If you want to read from it, you must ensure that you are in the same thread which the libgit2 function ran on.

I believe it has been pointed out that the current CLR behaviour is to keep using the same thread across a native function invocation, so we're not actually preventing a switch. However I do not think that this behaviour is specified, which means it could change at any point (though I wouldn't be surprised if it had to stay this way due to compatibility with programs which assume too much), and mono may not behave this way.

[nulltoken]

Related in-depth discussion: #225

[carlosmn]

Ah yes, that's what I was thinking about.

Since the main implementations agree that "a thread is a thread" then I suppose it's not harmful if we kill this and say we only support systems in which the same thread is used when going across the native bridge.

[carlosmn]

But if one of these major platforms suddenly decide to change, we're going to be in trouble.

[nulltoken]

@jamill @ethomson Thoughts?

[ethomson]

I think that there are three issues here:

1. Should we use Thread.BeginThreadAffinity() and Thread.EndThreadAffinity().
   Practically speaking, no. It was sort of a neat idea for SQL server's needs that turned out to be abandoned and we're still paying a tax on it.

   As @carlosmn points out, we'd be in bad shape if we were to run in a theoretical environment that could move a Task from thread to thread, but we're trying to fix that problem without it being well defined. For example: my reading of the docs for BeginThreadAffinity are that it guarantees that a single .NET thread will not be moved across operating system threads. But it does not appear to guarantee that multiple .NET threads will not be executed on a single operating system thread (like how the Java VM could use green threads back in the olden days.) Now given that their example is about critical sections, it seems unlikely, but if that isn't guaranteed then this would cause us similarly terrible problems fetching our TLS data.

   So even if thread affinity is necessary here, we don't know that it's sufficient, so paying a tax for something that we don't really know if it will work or not is unfortunate.

   However, we could keep the existing patterns in Proxy and make ThreadAffinity() determine if it's running in a user-mode scheduler and simply return an IDisposable that doesn't bother to do anything in the common case.

   But I think that figuring out how to reliably determine whether we're in a UMS probably requires finding a user-mode scheduler, which probably means writing a user-mode scheduler, and I suspect that we all have better uses of our time.

2. Our usage is probably incomplete

   Realistically, we only use it to call giterr_last() immediately after some call that fails to get the TLS error code. Do we need to put it around all our uses of SafeHandles? Can I really call git_branch_iterator on one OS thread and git_branch_iterator_free on another? I literally don't know, but I haven't thought critically about every SafeHandle and whether all operations on them could be interleaved across threads.

3. Making our IDisposable DisposableThreadAffinityWrapper easier to use.

   What it does and why, when it is necessary is not obvious from the name. Even if you start going spelunking through Proxy.cs you will not figure it out. Even if you google Thread.BeginThreadAffinity it will not be obvious why you would want to bind a .NET thread to an OS thread. You have to sew a couple of unrelated ideas together (ThreadAffinity is only used when we call Ensure.xxxResult) to figure it out, or have a pretty good understanding of how libgit2 does error handling.

   We could probably do a lot for the people that aren't in Proxy.cs very often by renaming some things around. I include myself in this list of people, I frequently forget why ThreadAffinity is necessary when I have to get in there.

[ethomson]

Also, a comment about the issue metadata itself: this issue has the stabilization tag, but it's not about consumer-facing API stabilization. If we're thinking about this as a "push to 1.0" issue then I understand the tag, but it would be nice if we had two of these tags: one for API stabilization, one for other major milestone-related cleanup. Just so that I can scan a list and see which is which at a glance without overtaxing my puny low-memory brain trying to remember.

[nulltoken]

Label stabilization has been dropped