Add Dispose/IDisposable to GCHandle

[Sergio0694]

Follow up from @jkotas's comment: #19459 (comment). The same issue also contains an extensive conversation about this proposal, with some practical use case scenarios as well (in particular, see this comment by @jkoritzinsky: #19459 (comment)).

Proposed API

namespace System.Runtime.InteropServices
{
      public struct GCHandle
+        : IDisposable
      {
+        public void Dispose();
      }
}

Usage Examples

// Given...
[DllImport("nativelib")]
public static unsafe extern void DoCallback(IntPtr state, delegate*<IntPtr, void> callback);

[UnmanagedCallersOnly]
public static void Callback(IntPtr state)
{
    GCHandle handle = GCHandle.FromIntPtr(state);
    object state = handle.Target;
}

// ...this PR would enable this
using (GCHandle handle = GCHandle.Alloc(state))
{
    DoCallback(handle, &Callback);
}

Risks

Copying/boxing a GChandle value and calling Dispose on each copy will still possibly result in a crash. This is the same behavior as other approved APIs already though (eg. DependentHandle), and it's a limitation with C# not having proper move semantics.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[AaronRobinsonMSFT]

Based on the conversation in #19459 I think there is enough to move onto an API review.

[stephentoub]

I assume the behavior / implementation is just:

public void Dispose() => Free();

[AaronRobinsonMSFT]

@stephentoub I think it would check if the handle was allocated first.

public void Dispose()
{
    if (IsAllocated)
        Free();
}

[stephentoub]

Yup. Thanks.

[jkotas]

public void Dispose()
{
   if (IsAllocated)
       Free();
}

This implementation is both not thread safe (you cannot safely call it from multiple threads and assuming first one wins) and slower than necessary (pays extra for the interlocked compare change inside Free).

I think we would either want thread safe implementation; or non-thread safe fast implementation (w/o interlocked compare exchange).

[stephentoub]

I think we would either want thread safe implementation; or non-thread safe fast implementation (w/o interlocked compare exchange).

Yes, sorry, I was just trying to ask about the semantics of what Dispose did, rather than actually implement it in a comment.

From a thread-safety perspective, I think it'd be really confusing/error-prone if Free and Dispose had different thread-safety guarantees, so my preference would be to have Dispose use CompareExchange, e.g.

IntPtr handle = Interlocked.Exchange(ref _handle, IntPtr.Zero);
if (handle != IntPtr.Zero)
    InternalFree(GetHandleValue(handle));

[MichalStrehovsky]

Is IDisposable on a struct really a good idea?

As a quick poll:

var m = MyStruct.Alloc();
using (m)
{
}
Console.WriteLine(m);

struct MyStruct : IDisposable
{
    private bool _isUsed;

    public static MyStruct Alloc() => new MyStruct { _isUsed = true };

    public void Dispose()
    {
        _isUsed = false;
    }

    public override string ToString()
    {
        return $"IsUsed: {_isUsed}";
    }
}

People whose intuition says this is going to print IsUsed: true, can you give this comment a "Laugh" reaction? If you think this will print IsUsed: false, can you give this comment a "Confused" reaction?

[stephentoub]

Is IDisposable on a struct really a good idea?

We do it in other advanced places that have a similar potential for misuse, e.g.
https://docs.microsoft.com/en-us/dotnet/api/system.buffers.memoryhandle.dispose?view=net-5.0

[jkotas]

From a thread-safety perspective, I think it'd be really confusing/error-prone if Free and Dispose had different thread-safety guarantees,

Note that the thread-safety guarantee in Free is nearly useless. I have not ever seen a code that would take advantage of it - such code would have to catch the InvalidOperationException that gets thrown when the handle has been freed already.

[stephentoub]

Note that the thread-safety guarantee in Free is nearly useless

If we really believe it's useless, then we should remove it from both.

My point is that I think Free and Dispose should make the same guarantees.

[terrajobst]

• Looks good as proposed

namespace System.Runtime.InteropServices
{
    public struct GCHandle : IDisposable
    {
        public void Dispose();
    }
}

[AaronRobinsonMSFT]

Given some of the fallout from #55596, I think we need to consider the impact of this change. Moving back to API needs work.

[Neme12]

I don't think the issue with xunit should block this API. This could happen when any type whatsoever starts implementing IDisposable. And it's only in tests - users can change their code. This issue wouldn't come from a library.