-2

I understand that we should use async and await when accessing a database as it takes time to process this request, and we want the program to get on with other tasks while it waits.

However, I'm not sure WHERE to make this await call.

Should the high-level module calling the low-level database access module use await, or should the low-level module use it?

I'm thinking that it should be the high-level module, as that has more stuff to do???

Some example code is below, right or wrong place to use the command?

High-level controller module:

public async Task<IActionResult> Delete(int id, string email)
{
    email = email.ToLower();

    IUsers user = new Users();
    
    // Async Await call here or in lower level module???
    bool del = await Task.Run(() => user.DeleteUser(id, email));                

    if (del == true)
    {
        System.Diagnostics.Debug.WriteLine(String.Format("User Deleted"));
        return Ok();
    }
    else
    {
        // If del is false or null
        System.Diagnostics.Debug.WriteLine("Error. User ID: {0} Email: {1} didn't match, not deleted. ", id, email));
        return BadRequest();
    }
}

Low-Level Database Access Module:

// Connection String
IDbConnection dbConnection = new MySqlConnection(@"Server=127.0.0.1;blah;");


bool IUsers.DeleteUser(int id, string email)
    {
        using (dbConnection)
        {
            try
            {
                string cmd = @"DELETE from users WHERE ID=@Id AND EMAIL=@Email";

                bool result = false;

                // Should async await maybe be here???
                var del = dbConnection.Execute(cmd, new { Id = id, Email = email });

                //Change result to True if execute command went ahead
                result = del > 0;

                // Returns True or False
                return result;
            }
            catch (Exception ex)
            {
                Console.WriteLine("Exception in Users.DeleteUser");
                Console.WriteLine("Exception: {0}", ex.Message);
                return false;
            }
        }
    }

Thanks for the feedback!

Scottish Smile
  • 455
  • 7
  • 22
  • 1
    Once you start using async/await you should use it from the bottom all the way to the top. So you should make `DeleteUser` return `Task` and await that, instead of doing `Task.Run`. And you should `await` an `ExcuteAsync` method or equivalent. – juharr Apr 05 '21 at 02:41
  • 1
    I recommend reading [this question](https://stackoverflow.com/questions/33027364/how-are-asynchronous-i-o-methods-processed). Someone else can undoubtedly give a more coherent answer than I could, but in short: use async methods for IO (e.g. `ExecuteAsync`) because this allows the task execution to be suspended while IO happens (freeing up resources to handle other requests). Using `Task.Run` in the way you are is simply locking up a thread on the thread pool that tasks use while you wait for your 100% synchronous DB code to execute, meaning you make things worse overall rather than better. – ProgrammingLlama Apr 05 '21 at 02:45
  • 4
    Async all the way top-bottom / bottom-top is the way. – ProgrammingLlama Apr 05 '21 at 02:49
  • Take a look at this: [Should I expose asynchronous wrappers for synchronous methods?](https://devblogs.microsoft.com/pfxteam/should-i-expose-asynchronous-wrappers-for-synchronous-methods/) The answer is no. Which means that if you really have to use `Task.Run`, you should use it at the last possible moment. – Theodor Zoulias Apr 05 '21 at 03:02

1 Answers1

2

In most cases, you should not use Task.Run on ASP.NET at all. In this case in particular, you should definitely not use Task.Run, because it's just running synchronous code on a background thread - what I call "fake asynchronous".

I recommend starting using async at the lowest level, and working your way up from there. In this case:

var del = await dbConnection.ExecuteAsync(cmd, new { Id = id, Email = email });

Then the compiler will give you an error saying you should mark DeleteUser as async and change its return type to Task<bool>. You'll also need to change the IUsers interface to do this:

async Task<bool> IUsers.DeleteUserAsync(int id, string email)

and update everything that calls it, leaving your controller method looking like:

bool del = await user.DeleteUserAsync(id, email);
Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810