Asynchronous wrapper for database connection

Question

I'm using a database that has a library for access to it. Connection is done synchronously, it's blocking and I do not have a way to abort it. To connect, I have to write:

var _loader = new DatabaseDriverLoader();
DatabaseDriver _driver = _loader.Connect(connectionString, login, password);

I wanted to improve it:

• Be able to connect asynchronously (not waiting for actual connection)
• Be able to cancel the connection if I need to

Here's a wrapper that I created:

public class DatabaseConnection : IDisposable
{
    private DatabaseDriver _driver;
    private DatabaseDriverLoader _loader

    public event EventHandler ConnectionFailed;
    public event EventHandler ConnectionEstablished;
    public event EventHandler ConnectionLost;

    public async Task ConnectAsync(string connectionString, string login, string password, CancellationToken cancellationToken)
    {
        var connectionTask = Task.Run(() =>
        {
            try
            {
                var _loader = new DatabaseDriverLoader();
                DatabaseDriver _driver = _loader.Connect(connectionString, login, password);
            }
            catch (Exception e) { }
        });

        await Task.Run(async () =>
        {
            while(!connectionTask.IsCompleted && !cancellationToken.IsCancellationRequested)
            {
                await Task.Delay(100);
            }
        });

        cancellationToken.ThrowIfCancellationRequested();

        if (_driver == null)
        {
            ConnectionFailed?.Invoke(this, EventArgs.Empty);
            return;
        }
        ConnectionEstablished?.Invoke(this, EventArgs.Empty);

        _driver.ConnectionToServerLost += OnConnectionLost;
    }

    public void Dispose()
    {
        if (_driver != null)
        {
            _driver.ConnectionToServerLost -= OnConnectionLost;
            _driver.Dispose();
        }
        _loader?.Dispose();
    }

    public DatabaseDriver GetDriver()
    {
        return _driver;
    }

    private void OnConnectionLost(object sender, EventArgs e) => ConnectionLost?.Invoke(sender, e);
}

Here's how I would use that class:

private async Task ConnectWithDatabase() 
 {
  //Delete old connection instance
  _dbConnectionTokenSource?.Cancel();
  if (_dbConnection != null)
  {
      _dbConnection.ConnectionEstablished -= OnDbConnectionEstablished;
      _dbConnection.ConnectionLost -= OnDbConnectionLost;
      _dbConnection.ConnectionFailed -= OnDbConnectionFailed;

      _dbConnection.Dispose();
  }

//Create new connection instance
  _dbConnection = new DatabaseConnection();
  _dbConnection.ConnectionEstablished += OnDbConnectionEstablished;
  _dbConnection.ConnectionLost += OnDbConnectionLost;
  _dbConnection.ConnectionFailed += OnDbConnectionFailed;

  _dbConnectionTokenSource = new CancellationTokenSource();
  var token = _dbConnectionTokenSource.Token;

  try
  {
      await _dbConnection.ConnectAsync(
          ConnectionString,
          Login,
          Password,
          token)
      .ConfigureAwait(false);
  }
  catch (TaskCanceledException e)
  { }
 } 

I wanted to ask you kindly for review of this code, because I'm not really sure if this is done correctly. I don't know if it is a good idea to use an event for successful connection. Maybe Connect method should return successful Task when connection occurs?

I'm not sure if cancellation is done correctly. In my use case there might be situations where DatabaseConection is canceled and disposed before connection occurs successfully.

I am not sure if it's a good idea to create new threads in DatabaseConnection, because clients of this class might not want it? - at least I read somewhere that it's not a good design, but I'm not sure why.

I will be happy to hear any opinion, always good to learn something new.

Answer 1

I would first start by saying that such a class is generally regarded as a bad practice because it doesn't achieve what's supposed to do, or simply there are better ways of doing it.

But let's go to the actual code, in the ConnectAsync method.

var connectionTask = Task.Run(() =>
{
    try
    {
        var _loader = new DatabaseDriverLoader();
        DatabaseDriver _driver = _loader.Connect(connectionString, login, password);
    }
    catch (Exception e) { }
});

Here you delegate the connection to the main class and initialize the _driver field. The first problem is that, since it's wrapped in a Task, it uses a separate thread and that new thread is the blocked one instead of the calling one. In a desktop program, it can be a good thing (since it keeps the UI thread free to handle the UI), but in a web site you free the original thread and stall another one, which is what you want to avoid with an async method.

Even worse, the empty catch block will swallow any exceptions, so you won't get any clue why a connection fails if that happens.

The next part also has its problems:

await Task.Run(async () =>
{
    while(!connectionTask.IsCompleted && !cancellationToken.IsCancellationRequested)
    {
        await Task.Delay(100);
    }
});

This employs a second thread and loops until the first one finishes or the cancellation is requested. While it sounds neat in principle, it actually ends up using double the resources it should for the ability to "cancel" the connection (more on that latter), which is far suboptimal. Callers expect NO threads consumed when you see an async DB connection, but instead this code stalls 2 of them.

What to do instead?

In short, leave the decision to the caller! This class introduced some side effects that callers normally don't expect. In web pages, it consumes far more resources than necesary, and in desktop programs it can be simply invoked via Task.Run method directly, instead of wrapping in a class, if keeping the UI responsive is desired. This removes the need for this class, using the provided driver directly.

About your other points

Be able to connect asynchronously (not waiting for actual connection)

It clearly does that, but I would simply use the first chunk of code (awaiting for it) directly, and remove the need for this, because of unwanted side effects.

Be able to cancel the connection if I need to

Here is the bigger catch. It does NOT cancel the connection at all! When the cancellation token is triggered, the loop is interrupted, but the first thread is left running, and its result is ignored. This means that if the connection succeeds, you have a floating connection that won't be disposed of properly, and the involved resources will still be consumed. The only practical effect of cancellation is the early return.

Maybe Connect method should return successful Task when connection occurs?

Yes, that's a good practice. I don't see a particular use for an event here (not that it's a bad thing to have one, but it serves little purpose). Most important, in its current form, the code does return a task and completes it on sucessful connection, and that's what you use in your "demo" code in fact. That's a good thing.