0

I am writing a class -- let's call it MessageSender -- that needs to perform operations over the network. It basically does these things:

  1. Take some configuration
  2. establish a connection
  3. send stuff

If we ignore the cleanup of any resources, this would look like this:

var sender = new MessageSender("127.0.0.1");
sender.Connect();
sender.SendMessage("Hello world");

The thing I am unsure about is how to manage the disposal of the established connection. I thought of three options, of which I ended up implementing the last one.

(1) Having a dedicated Disconnect() method the user must call:

var sender = new MessageSender("127.0.0.1");
sender.Connect();
sender.SendMessage("Hello world");
sender.Disconnect();

(2) The MessageSender implements IDisposable:

using (var sender = new MessageSender("127.0.0.1"))
{
    sender.Connect();
    sender.SendMessage("Hello world");
}

(3) The Connect() method returns an IDisposable:

var sender = new MessageSender("127.0.0.1");
using (var connection = sender.Connect())
{
    sender.SendMessage("Hello world");
}

I have never seen the third option anywhere, but it does seem to have some advantages:

  • Construction, and hence configuration of the message sender is separated from establishing a connection. E.g. the object itself can be some other classes' member, constructed and passed down as a dependency to others, while the execution and therefore the actual connection can be deferred to some Run() method.
  • The need for connection tear-down is a direct result from the connection set-up and cannot be (accidentally) separated.
  • If implemented that way, Connect() could be called multiple times.
  • Using IDisposable in general over a dedicated tear-down method gives you better language support, e.g. the using-clauses I used in both (2) and (3).

Potential pitfalls I see for all of the above solutions:

  • Failing to run the tear-down logik. This includes:
    • for (1): Forgetting to call Disconnect()
    • for (2) and (3): Forgetting to propery handle disposables
    • for (3): Ignoring the return value of Connect() entirely
  • MessageSender needs to keep track of its connection state to disallow multiple calls to Connect().
  • Calling SendMessage can fail at runtime depending on the current connection state.

Are there advantages to other approaches or disadvantages to my approach I am not aware of?

Here's a somewhat simplified version of my actual code:

public sealed class MessageSender
{
    private readonly Some3rdPartyNetworkClient _client;
    private bool _connected = false;

    public MessageSender(string connectionString)
    {
        _client = new Some3rdPartyNetworkClient(connectionString);
    }

    public void SendMessage(string message)
    {
        if (!_connected) throw new InvalidOperationException("not connected.");
        _client.SendMessage(message);
    }

    private sealed class DelegateDisposer : IDisposable
    {
        private readonly Action _dispose;
        public DelegateDisposer(Action dispose) => _dispose = dispose;
        public void Dispose() => _dispose();
    }

    public IDisposable Connect()
    {
        if (_connected) throw new InvalidOperationException("Can only ever connect once.");
        _connected = true;
        
        _client.Connect();
        var tokenSource = new CancellationTokenSource();
        Task checkConnectivityWorker = CheckConnectivityWorker(tokenSource.Token);
        return new DelegateDisposer(() =>
        {
            tokenSource.Cancel();
            if (!checkConnectivityWorker.IsCanceled) checkConnectivityWorker.Wait();
            _client.Disconnect();
        });
    }

    private async Task CheckConnectivityWorker(CancellationToken cancellationToken)
    {
        // some stuff that needs to be done continuously while the connection is active
    }
}
3
  • Why would it be advantageous to have a sender in an unconnected/invalid state? Is this sequential coupling between Connect() and SendMessage() methods worth it? Solution 2 looks best by far.
    – amon
    Commented Jul 12, 2020 at 15:17
  • In my current design I need to pass the message sender to dependents before establishing the connection. In fact, it is an interface IMessageSender that only describes the send method. Should I edit my question to add that detail?
    – Felk
    Commented Jul 12, 2020 at 16:01
  • 1
    #2 and #3 can be found in the CLR (e.g. SqlConnection and SqlDataReader, respectively).
    – John Wu
    Commented Jul 12, 2020 at 17:12

1 Answer 1

2

In .NET option 2 is the way to go. Option 3, although creative, only adds complexity in my eyes. The only reason to do something like that would be if setting up an instance of MessageSender would be expensive in some way. But then the less astonishing option would be to externalize the setup and pass an object that has the expensive stuff in it to MessageSender's constructor.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.