Problem disposing of socket / finalising twice?

Question

I'm working with some code (not mine I hasten to add, I don't trust this much at all) for a class which opens a socket, makes requests and listens for responses, which is throwing an exception in a way I can't comprehend when tested in xunit. I assume the same exception happens "live" but the class is referenced by a singleton so it is probably just hidden.

The problem manifests as "System.CannotUnloadAppDomainException: Error while unloading appdomain" in xunit and the inner exception is "System.ObjectDisposedException" thrown (essentially) inside the finaliser when closing the socket! There are no other reference to the socket which call close and dispose is protected on the Socket class so I'm not clear how else the object could be disposed.

Further, if I merely catch and absorb the ObjectDisposedException xunit terminates when it hits the line to close the listener thread.

I just don't get how the Socket can be disposed before it's asked to close.

My knowledge of sockets is only what I've learnt since finding this problem, so I don't know if I've provided everything SO might need. LMK if not!

public class Foo
{
    private Socket sock = null;
    private Thread tListenerThread = null
    private bool bInitialised;
    private Object InitLock = null;
    private Object DeInitLock = null;

    public Foo()
    {
     bInitialised = false;

     InitLock = new Object();
     DeInitLock = new Object();
    }

    public bool initialise()
    {
     if (null == InitLock)
      return false;

     lock (InitLock)
     {
      if (bInitialised)
       return false;

      sock = new Socket(AddressFamily.InterNetwork, SocketType.Dgram, ProtocolType.Udp);
      sock.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.MulticastTimeToLive, 8);
      sock.Bind( /*localIpEndPoint*/);
      sock.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.AddMembership, new MulticastOption(mcIP));

      tListenerThread = new Thread(new ThreadStart(listener));
      tListenerThread.Start();

      bInitialised = true;
      return true;
     }
    }

    ~Foo()
    {
     if (bInitialised)
      deInitialise();
    }

    private void deInitialise()
    {
     if (null == DeInitLock)
      return;

     lock (DeInitLock)
     {
      if (bInitialised)
      {
       sock.Shutdown(SocketShutdown.Both); //throws System.ObjectDisposedException
       sock.Close();

       tListenerThread.Abort(); //terminates xunit test!
       tListenerThread = null;

       sock = null;

       bInitialised = false;
      }
     }
    }
}

Answer 1

If this object is eligible for garbage collection and there are no other references to the Socket, then the socket's finalizer may well run before your object's finalizer. I suspect that's what's happened here.

It's generally a bad idea (IMO) to do this much work in a finalizer. I can't remember the last time I implemented a finalizer at all - if you implement IDisposable, you should be fine unless you have direct references to unmanaged resources, which are almost always in the form of IntPtrs. Orderly shutdown should be the norm - a finalizer should only usually run if either the program is shutting down, or someone has forgotten to dispose of the instance to start with.

(I know you clarified at the start that this isn't your code - I just thought I'd explain why it's problematic. Apologies if you already knew some/all of this.)

Answer 2

Because of the way the garbage collector and finalizers work, finalizers must only be used if your class is the direct owner of an unmanaged resource such as a Window Handle, a GDI object, a global handle or any other kind of IntPtr.

A finalizer must not try to dispose or even use a managed resource or you will risk calling a finalized or disposed object.

I highly recommend you to read this very important Microsoft article for more detail on how garbage collection works. Also, this is the MSDN reference on Implementing Finalize and Dispose to Clean Up Unmanaged Resources, look carefully for the recommendations at the bottom.

In a nutshell:

• If your object is holding an unmanaged resource, you should implement IDisposable and you must implement Finalizer.
• If your object is holding an IDiposable object, it should also implements IDisposable on it's own and dispose that object explicitly.
• If your object is holding both an unmanaged and a disposable, the finalizer must call two diferent version of Dispose, one that release disposable and unmanaged, the other only unmanaged. This is usually done using a Dispose(bool) function called by Dipose() and Finalizer().
• The Finalizer must never use any other resource than the unmanaged resource being released and self. Failing to do so will risk referencing collected or disposed objects, since an object is temporarily ressurected prior to finalization.

Answer 3

New info: this looks like I'm having two problems actually, but the thread one appears to be rather toxic.

From the MSDN link above:

"ThreadAbortException is a special exception that can be caught, but it will automatically be raised again at the end of the catch block."

Some very interesting community content also at that link including "Thread.Abort is a Sign of a Poorly Designed Program".

So at least I have some ammo to get this changed now :)