I am writing a class -- let's call it MessageSender
-- that needs to perform operations over the network. It basically does these things:
- Take some configuration
- establish a connection
- 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. theusing
-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
- for (1): Forgetting to call
MessageSender
needs to keep track of its connection state to disallow multiple calls toConnect()
.- 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
}
}
IMessageSender
that only describes the send method. Should I edit my question to add that detail?