5
\$\begingroup\$

The following code executes asynchron on the SQL database. Is the closing of the connection implemented in the right way? Are there any missing features regarding connection handling which need to be added?

public void deleteData(Node node, User user)
        {
            SqlConnection connection = getConnection();
            connection.Open();
            string query = "DELETE FROM NodeUserData WHERE node = @nodeId and userId = @userId;";
            SqlCommand cmd = new SqlCommand(query, connection);
            cmd.CommandType = CommandType.Text;

            try
            {
                cmd.Parameters.AddWithValue("@nodeId ", node.id);
                cmd.Parameters.AddWithValue("@userId", user.id);

                List<Task> asynctasks = new List<Task>();
                asynctasks.Add(cmd.ExecuteNonQueryAsync());
                Task.Factory.StartNew(() => closeConnectionOnFinish(asynctasks, connection));

            }
            catch (Exception ex)
            {                
                writeExceptionToLogFile(ex);
            }       
        }

And the closeConnectionOnFinish:

public async  void closeConnectionOnFinish(List<Task> _tasks, SqlConnection connection)
        {
            try
            {
                await Task.WhenAll(_tasks);
                connection.Close();
            }
            catch (Exception ex)
            {
                writeExceptionToLogFile(ex);
            }
        }
\$\endgroup\$

1 Answer 1

7
\$\begingroup\$

No that isn't right. If your query throws an error you won't be closing the connection. Swallowing exceptions like that (even though you're logging it) is generally bad as well. How does the user know anything has gone wrong?

If you can, just use async and await with a using:

public async Task DeleteDataAsync(...)
{
     using (var connection = GetConnection())
     using (var cmd = /* ... */)
     {
         // ... trimmed
         await cmd.ExecuteNonQueryAsync();
     }
}

If you can't do that (I don't know why you couldn't) then you could do this:

cmd.ExecuteNonQueryAsync().ContinueWith(_ => connection.Close());

But you should be waiting for that task to finish.


Your naming is non standard: methods should be PascalCase.


async void is generally bad - there's no way for the caller to wait until the work is finished. You shouldn't use async void methods (except for event handlers).


Async methods should have the suffix Async and return a Task or Task<T>.


Don't use Task.Factory.StartNew unless you want to specify advanced options; use Task.Run. However, don't use Task.Run for fake asynchrony - you'll just slow things down.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks so far. The code is executed on the server side of a game, so users (players) should not get error messages and response time should be as fast as possible - that's why I didnot wanted to use await and give errors to the user. Having a stable server would be more important than some millisecond gained in response time, so I will probably use the await solution. \$\endgroup\$
    – A. K-R
    Feb 22, 2016 at 15:57
  • \$\begingroup\$ Just saw that your example is itself an async method, so I suppose I can call it and be on the safe side regarding the connection, and do not inevitably have to wait for the response. \$\endgroup\$
    – A. K-R
    Feb 22, 2016 at 16:14

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.