-3

I'm writing a class library which includes socket operations. I don't want to rely on the consumer to dispose resources after finishing or when an exception is thrown.

Normally I would use the "using" statement, but this doesn't work outside of a local scope. In my case I'm having a class and am using the Socket in many public methods. It is easy to dispose the Socket when the connection ends, but how do I dispose it when exceptions are thrown without the "using" block?

Here is what I came up with: Basically I'm encapsulating every method the user has access to with a try catch block. If an exception happens I dispose the resources before the exception is thrown to the user.

But I'm wondering if there is a better alternative. And if it is bad if I hide this automatic disposing from the library consumer.

public class Foo
{
    Socket _socket;

    public void method1()
    {
         try
         {
              // some work with _socket
         }
         catch
         {
             _socket.Dispose();
             throw;
         }
    }
}
Josh
  • 287
  • 1
  • 8
  • 6
    Yes, you should rely on your consumer to dispose it. You signal this to your consumer by having your class implement IDisposable itself. That will give your consumer a warning if he fails to dispose your object, assuming he's using FxCop or similar tooling – Jonas Høgh Nov 12 '19 at 11:55

2 Answers2

4

You have 2 alternatives:

Either using standard IDisposable implementation

  public class Foo : IDisposable {
    Socket _socket;

    ...

    public void method1() {
      // working with _socket
    }

    protected virtual void Dispose(bool disposing) {
      if (disposing) {
        if (_socket != null) {
          _socket.Dispose();

          _socket = null;
        }  
      }
    }  

    public void Dispose() {
      Dispose(true); 
    }
  }

Or turning _socket into local variable:

public void method1() {
  using (Socket _socket = new Socket(...)) {
    // working with _socket
  }
}
Dmitry Bychenko
  • 180,369
  • 20
  • 160
  • 215
0

Starting from what's wrong

You should for example create the socket in the constructor and dispose it in the destructor, else after the first exception catched like in method1, you will have a reference to a disposed object that you can continue to use...

public class Foo
{
  private Socket _socket { get; }

  public Foo()
  {
    _socket = new Socket();
  }

  ~Foo()
  {
    _socket.Dispose();
  }

  public void method1()
  {
    try
    {
      // some work with _socket
    }
    catch
    {
      throw;
    }
  }
}

Doing things right

Ideally, you should mark Foo as implementing IDispose and dispose the socket in the Dispose method.

It will be more clean and robust.

So you can use Foo with the using pattern, or you can call Foo.Dispose.

public class Foo : IDisposable
{
  private Socket _socket { get; }

  private bool IsDisposed;

  public Foo()
  {
    _socket = new Socket();
  }

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

  protected virtual void Dispose(bool disposing)
  {
    if ( IsDisposed ) return;
    if ( disposing )
    {
      if ( _socket != null )
        try
        {
          _socket.Dispose();
          _socket = null;
        }
        catch ( Exception ex )
        {
          // Show message or do nothing
        }
    }
    IsDisposed = true;
  }
}

Implementing a Dispose method

  • 2
    @OlivierRogier I'd strongly suggest you remove the first code block. A finalizer is _definitely_ the wrong tool here (it is not safe to `Dispose` of a field from a finalizer like that, plus it is unnecessary since the `Socket`'s finalizer will take care of it at the same time anyway). The second code block is fine (although it would be nice if it used the standard pattern like Dmitry's). I've _never_ had to write a finalizer. https://devblogs.microsoft.com/cbrumme/finalization/ and http://joeduffyblog.com/2005/04/08/dg-update-dispose-finalization-and-resource-management/ may be worth a read. – mjwills Nov 12 '19 at 12:03
  • Sorry, I was unclear. You really should remove `~Foo() { _socket.Dispose(); }` **It is unsafe**. cbrumme's article discusses _why_ it is unsafe. – mjwills Nov 12 '19 at 12:09