1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @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. \$\endgroup\$ – Loreno Mar 7 '19 at 7:30
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – t3chb0t Mar 7 '19 at 7:37
  • \$\begingroup\$ @t3chb0t Modifications were about names and very small refactoring. After fixing, my code is same as I posted here. \$\endgroup\$ – Loreno Mar 7 '19 at 10:20
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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