12

Given the following code, I have a few questions about best practices:

string connectionString = @"Server=(local)\sqlexpress; Database=master; Integrated Security=true;";

using (SqlConnection connection = new SqlConnection(connectionString))
{
    using (SqlDataAdapter dataAdapter = new SqlDataAdapter("select * from information_schema.columns", connection))
    {
        await connection.OpenAsync();

        DataTable dataTable = new DataTable();
        await Task.Run(() => dataAdapter.Fill(dataTable));
        return dataTable;
    }
}

I've seen several examples that wrap the whole block of code in a Task.Run() call, but I'm not sure if that's better than calling Task.Run() only for the DataAdapter.Fill() method, which feels more flexible and specific (only using await on async tasks).

Is the approach of calling Task.Run() on the Fill() method better than wrapping the whole code block?

Are there any negative side-effects to calling Fill() in Task.Run()? I'm thinking of something along the lines of losing call stack and/or exception information if Fill() has an error.

Is there a better way to write this in ASP.NET?

Adrian Anttila
  • 2,038
  • 5
  • 22
  • 25
  • 1
    Why not put *all* of that code inside an async method? That is, move the Task to encompass the entire SQL connection life-cycle including filling the DataTable. – user2864740 Aug 28 '14 at 22:04
  • 1
    Is this ASP.NET or GUI? – usr Aug 28 '14 at 22:14
  • @usr, I was assuming ASP.NET; is there a big difference here? – Adrian Anttila Aug 29 '14 at 01:14
  • @user2864740, the SqlConnection.OpenAsync() method handles making the database connection asynchronous; I'm not sure why the rest of the code would need to be inside a Task. – Adrian Anttila Aug 29 '14 at 01:15
  • @AdrianAnttila If moving *all* the code to a waitable Task method the OpenAsync would become a normal call. The connection would be opened/disposed within the task before the final (DataTable) result was available to await. – user2864740 Aug 29 '14 at 01:29
  • 1
    @AdrianAnttila Anyway, as far as exception handling see - http://stackoverflow.com/questions/5383310/catch-an-exception-thrown-by-an-async-method The Exception is maintained and propagated to code which *invokes* `await`, just as the Context. – user2864740 Aug 29 '14 at 01:34
  • 1
    Task.Run almost never helps in a web app. What exactly would it improve? – usr Aug 29 '14 at 09:24
  • 3
    It could improve performance of a request by working in parallel, and it also creates a highly concurrent server, since you don't have a lock on that request, the server can move on to accept other requests while that one is processing. So I believe you could have advantages in some scenarios when well used. – Oakcool Aug 29 '14 at 23:52

2 Answers2

1

In ASP.NET it almost never helps to use Task.Run. What exactly would it improve? It only introduces overhead.

That said, Fill will perform IO (draining the data reader) so you might want to call it asynchronously. Unfortunately, there is no async version of this method.

If you insist on using async IO (which is questionable for database access) you need to find an alternative. Maybe async Entity Framework or raw ADO.NET can help you.

Community
  • 1
  • 1
usr
  • 168,620
  • 35
  • 240
  • 369
0

Have you tried using a DataReader and the new ExecuteReaderAsync? What I recall is the SqlDataAdapter already uses a DataReader internally without the async. You may also want to skip using a DataTable altogether if possible to cut down on some overhead.

For small result sets that very rarely change such querying schema columns I would maybe just cache it on the web server in one of many ways. Heck, for schema changes you could even create a simple DDL trigger to update a single row table with a timestamp field to let you know a change has been made, then only run the query when necessary. Another option is CHECKSUM_AGG for tables other than schema tables.

John Kosh
  • 1