Correct way to avoid a ObjectDisposedException, in this case?

Question

I am currently getting an ObjectDisposedException on the following line.

var client = ((Socket) asyncResult.AsyncState).EndAccept(asyncResult);

System.ObjectDisposedException: 'Cannot access a disposed object. Object name: 'System.Net.Sockets.Socket'.'

I was just wondering, what is the right way (in terms of best practice) to avoid an error like this? I'm unsure on how to handle it, how do I check if its disposed before hand, but is that what I should be doing? Or checking something else.

I'm self taught C# so I never learnt things like this, could someone give some insight?

Here is the full class:

internal sealed class SocketHandler : IDisposable
{
    private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

    private readonly Socket _serverSocket;

    public SocketHandler()
    {
        _serverSocket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);

        Load();
    }

    public void Dispose()
    {
        _serverSocket?.Close(); // close also calls dispose...
    }

    private void Load()
    {
        var config = Program.Server.ConfigHandler;

        _serverSocket.Bind(new IPEndPoint(IPAddress.Any, config.GetConfigValueByKey("network.sockets.port").ToInt()));
        _serverSocket.Listen(int.Parse(config.GetConfigValueByKey("network.sockets.backlog")));
        _serverSocket.BeginAccept(OnAcceptConnection, _serverSocket);
    }

    private void OnAcceptConnection(IAsyncResult asyncResult)
    {
        try
        {
            if (_serverSocket == null)
            {
                return;
            }

            var client = ((Socket) asyncResult.AsyncState).EndAccept(asyncResult);

            var playerHandler = Program.Server.BaseHandler.PlayerHandler;
            var players = playerHandler.Players;

            var config = Program.Server.ConfigHandler;

            var maxConnections = int.Parse(config.GetConfigValueByKey("game.players.limit"));
            var maxConnectionsPerIp = int.Parse(config.GetConfigValueByKey("game.players.ip_limit"));

            if (players.Count >= maxConnections)
            {
                Logger.Warn("Incoming connection was refused because the player limit was exceeded.");

                client.Shutdown(SocketShutdown.Both);
                client.Close();

                return;
            }

            if (players.Values.Count(x => x != null && !x._disconnected && x.getIp() == client.RemoteEndPoint.ToString().Split(':')[0]) > maxConnectionsPerIp)
            {
                Logger.Warn("Incoming connection was refused because the IP limit was exceeded.");

                client.Shutdown(SocketShutdown.Both);
                client.Close();

                return;
            }

            var clientId = Randomizer.Next(1, 10000);

            Program.Server.BaseHandler.PlayerHandler.TryAddPlayer(clientId, new Player(clientId, client, new InitialPacketParser()));
        }
        catch (SocketException socketException)
        {
            Logger.Fatal(socketException, "Failed to accept socket connection.");
        }
        finally
        {
            _serverSocket?.BeginAccept(OnAcceptConnection, _serverSocket);
        }
    }
}

Answer 1

As far as I understand the reference source a Socket does not Dispose itself. So, since your _serverSocket is private, you are the only one to control when it is disposed.

Your OnAcceptConnection() method already started to try to check that, but not completely.

In your Dispose() method (or any other place where you Close() or Dispose() your _serverSocket) you need to also set _serverSocket to null. You can do it like this in a thread-safe way:

public class SocketHandler
{
    private Socket _serverSocket; // not read-only

    /* ... */

    public void Dispose()
    {
        Socket tmp = _serverSocket; // save instance
        _serverSocket = null; // set field to null
        tmp?.Close();
    }

Now your OnAcceptConnection() returns when it checks if (_serverSocket == null) and you avoid the exception.

Answer 2

The problem might also come from how you are using SocketHandler in the first place. I can see nothing wrong with how the IDisposable pattern is implemented in your class. The normal way to avoid accessing a Disposable class after it has been disposed is wrapping it in a using statement which will auto-dispose the class after leaving the block:

using( SocketHandler handler = new SocketHandler())
{
    (...)
} //handler will be disposed and not accessible after here

For more information on this Microsoft Docs has a good article explaining IDispose

EDIT: Thanks for pointing out that I was on a completely wrong way to understand the question.

I at least recreated the scenario to best of my knowledge:

class Program
{
    static void Main(string[] args)
    {
        using (C1 instance = new C1())
        {
            Task.Factory.StartNew(() =>
            {
                Task.Delay(1000);

                bool disposed = (bool)typeof(C1).GetField("disposed", BindingFlags.NonPublic | BindingFlags.Instance).GetValue(instance);

                if (disposed)
                {
                    Console.WriteLine("Already disposed will not call DoSomething()");
                }
                else
                {
                    instance.DoSomething();
                }

            });
        }

        Console.ReadKey(true);
    }
}

class C1 : IDisposable
{
    bool disposed = false;
    public C1()
    {

    }

    public void DoSomething()
    {
        if (disposed)
            throw new ObjectDisposedException("C1");

        Console.WriteLine("Still existing!");
    }


    public void Dispose()
    {
        Dispose(true);

        Console.WriteLine("Disposed!");
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposed)
            return;
        disposed = true;
    }
}

And managed to workaround the exception with a reflection. Assuming that microsoft will have used the same pattern + namings as they are according to their Framework Design Guidelines this solution could at least be used as a workaround.

But I highly doubt that this the best way.