What do you think of my IDisposable pattern implementation?

Question

What do you think of the following IDisposable pattern implementation?

public class Connection : IDisposable 
{
    private Socket _socket;

    public bool IsConnected()
    {
        if (_socket.Poll(1, SelectMode.SelectRead) && _socket.Available == 0)
            return false;
        return true;
    }

    public void Disconnect()
    {
        if (m_socket != null && IsConnected())
        {
            try
            {
                _socket.Shutdown(SocketShutdown.Both);
                _socket.Disconnect(false);
            }
            catch (SocketException se)
            {
                System.Console.WriteLine(se.Message);
            }
        }
    }

    ~Connection()
    {
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!IsConnected())
        {
            if (disposing)
            {
                Disconnect();
            }
            else
            {
                AppDomain currentDomain = AppDomain.CurrentDomain;
                if (currentDomain.IsFinalizingForUnload() && !Environment.HasShutdownStarted)
                {
                     System.Console.WriteLine("Client failed to call Destroy");
                }
            }
        }
    }
}

I was given this error using the code above:

{"An operation was attempted on something that is not a socket"} System.Net.Sockets.Socket.Poll(Int32 microSeconds, SelectMode mode)

Answer 1

The implementation is severely flawed. You don't truly implement IDisposable, and you end up relying on the garbage collector to clean up your resources, which is a bad thing.

Additionally, you don't even clean up those resources properly when the GC does come around (it does do it correctly, but it's by mistake that it happens).

It is the responsibility of your class to implement IDisposable as you are holding onto resources which implement IDisposable. Then, in your implementation of Dispose, if you are not being GCed (it is an explicit call to Dispose) you are to call Dispose on any IDisposable implementations that you are holding onto.

You check the connection status of the Socket, but that's not the same as calling Dispose on it, and you leak the resource as a result (GC eventually picks it up).

For the guideline on how to properly implement IDisposable, see the section of the MSDN documentation titled "Implementing Finalize and Dispose to Clean Up Unmanaged Resources", located here:

http://msdn.microsoft.com/en-us/library/b1yfkh5e(VS.71).aspx

I should note that I don't agree completely with these guidelines, but they are the most adopted. For my position, see here:

http://www.caspershouse.com/post/A-Better-Implementation-Pattern-for-IDisposable.aspx

Answer 2

For starters, why aren't you implementing IDisposable?

That would give a clear indication to users of your object that they actually need to dispose of it when they're done. They could also then wrap it in a using block etc.

Answer 3

This what I have used in the past for a Disposable pattern (I'd suggest starting with this):

public class Connection : IDisposable
{
    #region Dispose pattern

    /// <summary>
    /// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
    /// </summary>
    /// <filterpriority>2</filterpriority>
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    private void Dispose(bool disposing)
    {
        ReleaseUnmanagedResources();
        if (disposing)
            ReleaseManagedResources();
    }

    /// <summary>
    /// Derived classes from this class should override this method
    /// to clean up managed resources when Dispose is called.
    /// </summary>
    protected virtual void ReleaseManagedResources()
    {
        // Enter managed resource cleanup code here.
    }

    /// <summary>
    /// Derived classes should override this method to clean up
    /// unmanaged resources when Dispose is called.
    /// </summary>
    protected virtual void ReleaseUnmanagedResources()
    {
        // Enter unmanaged resource cleanup code here.
    }

    #endregion

Answer 4

I just want to add this to the pattern pointed by Richardson

private bool disposed;

private void Dispose(bool disposing)
{
    if(!this.disposed)
    {
        ReleaseUnmanagedResources();
        if (disposing)
            ReleaseManagedResources();
        this.disposed = true;
    }
}

Answer 5

This implementation is flawed, for a few reasons.

First, your Dispose() method should have a single purpose - to call socket.Dispose();. Right now, you are putting far too much logic in there, and not actually "Disposing" of the single managed, IDisposable resource you own.

Second, you do not need a finalizer at all, since you do not directly own or allocate any native, unmanaged resources. The only resource that you are disposing of is a Socket, which is managed, and will implement its own finalizer as needed. If you want to trap and find the cases where a Connection was not disposed properly, I would set up a debug-only finalizer to warn of that case. The finalizer in IDisposable is meant to handle the case where the GC must do the cleanup because the caller forgot to call Dispose() - in your case, the socket's finalizer will take care of that for you.

Third, part of the IDisposable pattern as suggested in the design guidelines from Microsoft states that the client should be able to call Dispose() multiple times with no consequences. There should be nothing using the socket directly after the first call to Dispose() - in fact, I would suggest that Dispose() should call socket.Close(); or (socket as IDisposable).Dispose(); and immediately set socket = null; to prevent this from being a possibility. With your current logic, it is very possible to have the calls in IsConnected() cause the socket to throw an exception on subsequent calls to Dispose(), which should be avoided.

Fourth, it's a strong suggestion to have a Close() method on all resources that use a file, socket, or other "closable" resources. Close() should call Dispose().

Finally, there should be checks post-disposal for use of the Connection. Any method on the connection used after disposal should throw an ObjectDisposedException.

Answer 6

This doesn't appear to be explicitly stated in the other answers, so if you're interested in what specifically is going wrong, here it is.

When two objects have finalizers, and are reclaimed by the GC, there is no particular order in which their finalizers are executed. In your code, the Socket class has a finalizer, and your class has a finalizer. If the finalizer is executed on the Socket instance first, then when your finalizer executes, you will be attempting to call methods on an object that has been finalized, hence the exception.

Finalizers basically suck. You hardly ever need to write one (even if you are dealing with raw Win32 handles - use SafeHandle instead.)

Instead, just implement IDisposable, and don't write a finalizer.

Answer 7

Finalizers are a pain, and you should avoid them being called if possible. However if you are implemented IDisposable, you will almost always want to write one (in conjunction with a Dispose(bool) method). That calls Dispose(false) and GC.SuppressFinalize().

If you don't implement a finalizer, then unless any IDisposable instances that you are holding implement finalizers correctly, you will leak. Say for example that SafeHandle didn't have a finalizer but relied on someone calling Dispose? If you held one, and didn't implement a finalizer, then you're allowing your consumer to permanently leak the handle by not calling Dispose on you!

Assuming that both your consumer and objects that you consume will play nice isn't a reason not to play nice yourself.

In addition, even if you use a SafeHandle, you should Dispose of it as if it were managed object. Otherwise if you don't dispose of it at all then it will have to wait until GC to be released, and if you do Dispose of it in Dispose but no a finalizer, and no one disposes of you, then it will have to wait until the GC after the one that collected you.

b_richardson above has the right pattern, it's not hard, and by adding GC.SuppressFinalize() you avoid the two-pass GC except in cases where no on disposed of you properly.

As for adding a boolean to track if we've been Disposed of yet, that'll work, I prefer to just write my Dispose methods to be rerunnable(check for null, etc.) also helps to do it that way when an object may acquire resources during use and you only want to dispose the ones it has.