-2

In a standard dispose/finalise pattern such as Finalizers with Dispose() in C# the Dispose(bool) does not touch managed objects if the method is called from the finalizer, it is considered unsafe as they may have already been collected by the garbage collector.

What is special about IntPtr etc that makes them safe?

As some background, to keep the cleanup code near the allocate code I'm adding the cleanup action to an event as soon as I allocate, then calling the event from the dispose method:

class TestClass : IDisposable
{
    private IntPtr buffer;

    public void test()
    {
        buffer = Marshal.AllocHGlobal(1024);
        OnFreeUnmanagedResource += (() => Marshal.FreeHGlobal(buffer));
    }

    private List<IDisposable> managedObjectsToBeDisposed = new List<IDisposable>();
    private event Action OnFreeUnmanagedResource = delegate { };
    private bool _isDisposed = false;

    private void Dispose(bool itIsSafeToAlsoFreeManagedObjects)
    {
        if (_isDisposed) return;
        OnFreeUnmanagedResource();

        if (itIsSafeToAlsoFreeManagedObjects) 
            for (var i = managedObjectsToBeDisposed.Count - 1; i >= 0; i--)
            {
                var managedObjectToBeDisposed = managedObjectsToBeDisposed[i];
                managedObjectToBeDisposed.Dispose();
                managedObjectsToBeDisposed.Remove(managedObjectToBeDisposed);
            }

        _isDisposed = true;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this); 
    }

    ~TestClass()
    {
        Dispose(false);
    }
}

I'm uncertain of this code because OnFreeUnmanagedResource may be collected before the class, but why would this not be the case for buffer?

Community
  • 1
  • 1
Stephen Turner
  • 7,125
  • 4
  • 51
  • 68
  • 1
    The premise of your question is totally false (all the first paragraph) – xanatos Feb 22 '16 at 11:22
  • An IntPtr usually stores a handle or an unmanaged pointer. A reference to a resource that neither the garbage collector or a friendly .NET wrapper knows anything about. So cleanup of the underlying unmanaged resource is not automatic and a finalizer is required to take care of it. And IDisposable if it is necessary to do it early enough. Note that this code does not qualify, it just allocates a bit of memory. Much better to use GC.AddMemoryPressure(). – Hans Passant Feb 22 '16 at 11:49
  • _it is considered unsafe as they may have already been collected_ is not true. It should be safe to Dispose() them but that is simply unnecessary. – H H Feb 23 '16 at 09:00

2 Answers2

1

With that anti-pattern (really you're better off either only having managed fields or only an unmanaged field rather than mixing them and then having to be clever about how that mixing is dealt with, but alas the pattern is still with us and has to be dealt with sometimes) the danger is not that disposable managed objects may have been collected (they won't be, they're being kept alive by the field in the class in question, and now at least rooted from that object in the finalisation queue, if not elsewhere) but that they may have been finalised by their own finaliser. Or conversely, that they might get finalised here and then finalised again because they are already in the finalisation queue.

If you'd arrived at this code via Dispose() then they would not (assuming no bugs, obviously) have been cleaned up because they only path prior to a collection attempt that can clean them is that very method.

If you'd arrived at this code via the finaliser then this object has had collection attempted on it, and been put in the finalisation queue, which means that likely the objects accessible only through it also had collection attempted on it, and if finalisable had been put in the queue, and there's no guarantee of which was put there first.

If the object was disposable but not finalisable, it quite likely had fields in turn that were finalisable and likewise are likely to be on that queue.

And if the object was disposable but not finalisable, and had no finalisable fields then it doesn't matter that you don't do anything with it.

Jon Hanna
  • 110,372
  • 10
  • 146
  • 251
0

This would be OK:

private void Dispose(bool itIsSafeToAlsoFreeManagedObjects)
{
    if (_isDisposed) return;
    Marshal.FreeHGlobal(buffer));

    if (itIsSafeToAlsoFreeManagedObjects) 
        for (var i = managedObjectsToBeDisposed.Count - 1; i >= 0; i--)
        {
            var managedObjectToBeDisposed = managedObjectsToBeDisposed[i];
            managedObjectToBeDisposed.Dispose();
            managedObjectsToBeDisposed.Remove(managedObjectToBeDisposed);
        }

    _isDisposed = true;
}

An IntPtr is a struct, that essentially contains a native handle. The resource refered to by the handle is not touched by the garbage collector. The struct itself is also still valid.

I'm not so sure about your code, where you use a managed object of reference type (the delegate attached to OnFreeUnmanagedResource) in the finalizer.

Edit: After reading the other answer, I think, your code is also OK, as the delegate doesn't have a finalizer.

Henrik
  • 23,186
  • 6
  • 42
  • 92
  • Sure, if I only have one buffer. If I have many I'd like to set the cleanup code as soon as I allocate the memory, rather than separate it in the Dispose(bool) method. – Stephen Turner Feb 22 '16 at 11:33