# Asynchronous wrapper for database connection

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();


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;

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

public async Task ConnectAsync(string connectionString, string login, string password, CancellationToken cancellationToken)
{
{
try
{
}
catch (Exception e) { }
});

await Task.Run(async () =>
{
{
}
});

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();
}
}

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,
token)
.ConfigureAwait(false);
}
{ }
}


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.

• @HenrikHansen Ofcourse, you are right. Mistakes came over, because I modified code a bit before posting here to remove unnecessary parts. I fixed it now. Async behaviour is needed, because I do not want thread to be blocked when I start connecting to database. – Loreno Mar 7 '19 at 7:30
• because I modified code a bit before posting here to remove unnecessary parts - there are no unnecessary parts on Code Review. Please post your actual code without modifying it. Otherwise you might not get the feedback you are hoping for and both reviewers and you will be unhappy about having wasted their time. – t3chb0t Mar 7 '19 at 7:37
• @t3chb0t Modifications were about names and very small refactoring. After fixing, my code is same as I posted here. – Loreno Mar 7 '19 at 10:20

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
{
}
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 () =>
{
{
}
});


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.