Runtime: Consider Implementing IDisposable in GCHandle

[Gnbrkm41]

(Pretty much duplicate of dotnet/runtime#26854, re-opening due to reason that is to be explained)

GCHandle is supposed to be freed once it goes out of scope: otherwise, memory leaks may occur. Although it has a Free() method which does that, it does not implement IDisposable and Dispose() method which lets it to be used in using blocks. Having it would be helpful since major .NET Languages such as C#, VB.NET, and F# provides syntactic sugar for using disposable types, using / use. I believe it's a pretty common practice for disposing of types that hold unmanaged resources.

The original issue, dotnet/runtime#26854, discusses about the exact same suggestion; however, the language feature proposal appear to have changed not to include this change for non-ref struct types, due to backwards-compat concerns.

[jcouv update:] This feature was restricted to ref structs due to backwards compatibility reasons. Also, the part about considering extension methods was split out of this feature, and it was not implemented in dev16.0 milestone.

This leaves the original problem unsolved. For that reason, I suggest that we reconsider implementing the interface on the type.

[Gnbrkm41]

(I am happy to open a PR incl. tests if this is approved)

[vcsjones]

This has been discussed for quite some time and part of the concern around doing this is it encourages developers to box the GCHandle, and boxing a GCHandle is problematic because the _handle inside of it is mutable, and boxing results in copying the struct.

If we pretended for a moment that GCHandle did implement the interface:

var obj = new object();
var handle = GCHandle.Alloc(obj);
Console.WriteLine(handle.IsAllocated);

((IDisposable)handle).Dispose(); //Box and dispose

Console.WriteLine(handle.IsAllocated);

The box and dispose here would duplicate the struct and IsAllocated would return true incorrectly on the second call.

[Gnbrkm41]

Oh LOL. Actually forgot the fact that it's a struct. silently closes the issue

[benaadams]

part of the concern around doing this is it encourages developers to box the GCHandle

Does that really matter? Its use is still guarded and this has same problem

var obj = new object();
var handle = GCHandle.Alloc(obj);
Console.WriteLine(handle.IsAllocated);

DoSomethingWithHandle(handle); //Pass by value and dispose

Console.WriteLine(handle.IsAllocated);

void DoSomethingWithHandle(GCHandle handle)
{
    handle.Free();
}

True

[Gnbrkm41]

Maybe I'll just let the people here to close it. We'll see what kind of response we'll get

[john-h-k]

While the boxing isn't ideal, the value type nature of GCHandle means you're going to get copies anyway, boxing is just one particular way. Particularly as this type already must be used with reasonable vigilance, I don't think adding the interface will hurt the use massively

[benaadams]

You can dupe the struct by setting it to another variable or pass by value; or set it to a readonly field; all of which will report that its allocated (readonly GCHandle probably being the worst); so don't think the risk of boxing is too big a worry?

[vcsjones]

Its use is still guarded

I'm not sure I follow.

of which will report that its allocated

IsAllocated is less of the concern over AddrOfPinnedObject. If you Free the GCHandle then AddrOfPinnedObject will throw an InvalidOperationException telling you the handle is not initialize.

If you free a copy of it then AddrOfPinnedObject is pointing to a bogus address. Best case you get an AccessViolationException, worst case you garbage data.

I agree it is fairly easy to cause a GCHandle to get duplicated. You can also box it to object which will have the same problem. Boxing via IDisposable is just yet another way of Your Holding It Wrong.

[benaadams]

If you free a copy of it then AddrOfPinnedObject is pointing to a bogus address.

The one that thinks its still allocated will throw an InvalidOperation "This handle is not pinned"; it checks with the GC before giving you the address:

Its use it guarded, regardless of what state the struct thinks its in

[vcsjones]

Its use it guarded, regardless of what state the struct thinks its in

Maybe that is new for 3 then? Here is what I get with .NET Core 2.2:

static void Main(string[] args)
{
    var bytes = new byte[] { 1 };
    var obj = GCHandle.Alloc(bytes, GCHandleType.Pinned);
    DumpAndFree(obj);
    DumpAndFree(obj);

    void DumpAndFree(GCHandle handle)
    {
        var ptr  = handle.AddrOfPinnedObject();
        Console.WriteLine(Marshal.ReadByte(ptr));
        handle.Free();
    }
}

Unhandled Exception: System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Runtime.InteropServices.Marshal.ReadByte(IntPtr ptr, Int32 ofs)
   at scratch.Program.<Main>g__DumpAndFree|0_0(GCHandle handle) in /code/personal/scratch/Program.cs:line 20
   at scratch.Program.Main(String[] args) in /code/personal/scratch/Program.cs:line 15

[vcsjones]

Ah, in your screenshot your GCHandle.Alloc isn't pinned. I don't think you can call AddrOfPinnedObject without initializing it as pinned in the first place.

[benaadams]

Ah... my mistake; I thought it did an additional check, but that's only in debug

#if DEBUG
        // The runtime performs additional checks in debug builds
        [MethodImpl(MethodImplOptions.InternalCall)]
        internal static extern object InternalGet(IntPtr handle);
#else
        internal static unsafe object InternalGet(IntPtr handle) =>
            Unsafe.As<IntPtr, object>(ref *(IntPtr*)handle);
#endif

[vcsjones]

Anyway, given that:

1. There are a million different ways to accidentally copy a GCHandle
2. The C# compiler can probably never change the behavior of omitting a box when in a using statement.

Perhaps it isn't too bad of an idea to implement.

[benaadams]

The C# compiler can probably never change the behavior of omitting a box when in a using statement.

It would be very bad for struct things if it did start boxing them. However it would be important not to be explicitly implemented; as that would only being downsides, other than the slightly nicer syntax.

[vcsjones]

You can still get in to trouble with using like:

GCHandle handle = // Get a handle
using (handle)
{

}

// handle would still be IsAllocated=true here

Which is admittedly a little weird. It seems using GCHandle handle for C# 8 using declaration is OK.

[benaadams]

😱 TIL using creates a struct copy and uses it in the finally block; but uses original in the try?

That's kinda weird?

static void Main(string[] args)
{
    GCHandlish handle = GCHandlish.Create();
    using (handle)
    {
        Console.WriteLine(handle.IsAllocated);
    }

    Console.WriteLine(handle.IsAllocated);
}

struct GCHandlish : IDisposable
{
    public bool IsAllocated { get; private set; }

    public static GCHandlish Create() => new GCHandlish() { IsAllocated = true };

    public void Dispose() => IsAllocated = false;
}

IL_0000:  nop         
IL_0001:  call        UserQuery+GCHandlish.Create
IL_0006:  stloc.0     // handle
IL_0007:  ldloc.0     // handle
IL_0008:  stloc.1     // handle copy
IL_0009:  nop         
IL_000A:  ldloca.s    00 // handle
IL_000C:  call        UserQuery+GCHandlish.get_IsAllocated
IL_0011:  call        System.Console.WriteLine
IL_0016:  nop         
IL_0017:  nop         
IL_0018:  leave.s     IL_0029
IL_001A:  ldloca.s    01 // handle copy 
IL_001C:  constrained. UserQuery.GCHandlish
IL_0022:  callvirt    System.IDisposable.Dispose
IL_0027:  nop         
IL_0028:  endfinally  
IL_0029:  ldloca.s    00 // handle
IL_002B:  call        UserQuery+GCHandlish.get_IsAllocated
IL_0030:  call        System.Console.WriteLine
IL_0035:  nop         
IL_0036:  ret         

[benaadams]

Is fine if you create it in the using though

static void Main(string[] args)
{
    using (GCHandlish handle = GCHandlish.Create())
    {
        Console.WriteLine(handle.IsAllocated);
    }
}

IL_0000:  nop         
IL_0001:  call        UserQuery+GCHandlish.Create
IL_0006:  stloc.0     // handle
IL_0007:  nop         
IL_0008:  ldloca.s    00 // handle
IL_000A:  call        UserQuery+GCHandlish.get_IsAllocated
IL_000F:  call        System.Console.WriteLine
IL_0014:  nop         
IL_0015:  nop         
IL_0016:  leave.s     IL_0027
IL_0018:  ldloca.s    00 // handle
IL_001A:  constrained. UserQuery.GCHandlish
IL_0020:  callvirt    System.IDisposable.Dispose
IL_0025:  nop         
IL_0026:  endfinally  
IL_0027:  ret