Monthly Archives: June 2019

Code reviews

This is a series about great code reviews, that I either gave or received. Code reviews are crucial for code quality and I strongly recommend you to have it in your company. Two heads are always better than one, especially in an atmosphere of cooperation and mutual learning. I have pleasure and luck to work in such a team and I have to admit, that code review discussions are always appreciated.

Without further due, let’s get to it. Have a good read! 🙂

Have you ever got a great review feedback? If you do, feel free to write me! I bet it’s worth to share it with the community and readers of this blog.

Code review #3 – use threads like Mr Fowler

This is another post in a series about great code review feedback, that I either gave or received. It will always consist of 3 parts: context, review feedback and explanation. You can go ahead and read the previous ones here: https://www.michalbialecki.com/2019/06/21/code-reviews/. This post is about doing things the right way.

The context

In my daily job we have a custom scheduling service, that is very simple. It just makes POST requests without a body based on a CRON expression. Then it check if the request was successful. The problem occured when jobs were getting longer and longer so that they reached a request timeout and apperad to be failed, while on a receiver, they run just fine. We decided, that a receiver should just return a success and do it’s work in a separate thread. This is how I approached this task:

Instead of having call with an await:

[HttpPost("ExportUsers")]
public async Task<IActionResult> ExportUsers()
{
    var result = await _userService.ExportUsersToExternalSystem();
    return result ? Ok() : StatusCode(StatusCodes.Status500InternalServerError);
}

I added a simple wrapper with Task.Run

[HttpPost("ExportUsers")]
public IActionResult ExportUsers()
{
    Task.Run(
        async () =>
            {
                var result = await _userService.ExportUsersToExternalSystem();
                if (!result)
                {
                    // log error
                }
            });
            
    return Ok();
}

Review feedback

Start a new thread as David Fowler does.

Explanation

When using Task.Run a thread is blocked from a thread pool. This is not a bad idea, but it should be used wisely. The thread should be released in a reasonable timeframe, not blocked for a lifetime of an application. An anti-pattern here would be to start listening Service Bus messages inside Task.Run, which would block a thread forever.

After refactoring my code looks like this:

[HttpPost("ExportUsers")]
public IActionResult ExportUsers()
{
    var thread = new Thread(async () =>
    {
        var result = await _userService.ExportUsersToExternalSystem();
        if (!result)
        {
            // log error
        }
    })
    {
        IsBackground = true
    };
    thread.Start();

    return Ok();
}

You can look at the more detailed explanation by David Fowler here: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/HEAD@%7B2019-05-16T19:27:54Z%7D/AsyncGuidance.md#avoid-using-taskrun-for-long-running-work-that-blocks-the-thread

There are many examples here that really makes you think about how to write better code, so I strongly encourage you to read it. Enjoy! 🙂

Code review #2 – remember about your awaits

This is another post about great code review feedback, that I either gave or received. It will always consist of 3 parts: context, review feedback and explanation. You can go ahead and read the previous ones here: https://www.michalbialecki.com/2019/06/21/code-reviews/. This time I’m going to show you a very simple bug I found.

The context

The problem occurred when I was investigating a code with multiple available calls. And after some refactoring, it came down to something like this.

public async Task InsertUser()
{
    try
    {
        // async stuff here

        client.PostAsync("http://localhost:49532/api/users/InsertMany", null).ConfigureAwait(false);

        // more async cally 
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
        throw;
    }
}

The thing was, that my call to InsertMany endpoint was not working somehow, and I didn’t catch any exceptions in this code. Do you know what’s wrong with it?

Review feedback

Oh, you haven’t awaited this one!

Explanation

That’s correct. There is a try-catch block that should catch every exception, but when async call is not awaited, then a Task is returned. So as a result no call would be made. This is potentially dangerous, because code will compile. IDE will give you a hint, but it can be easily overlooked.

As a good practice you can check if a call was a success:

public async Task InsertUser()
{
    try
    {
        // async stuff here

        var response = await client.PostAsync("http://localhost:49532/api/users/InsertMany", null).ConfigureAwait(false);
        response.EnsureSuccessStatusCode();

        // more async cally 
    }
    catch (Exception e)
    {
        Console.WriteLine(e);
        throw;
    }
}

So take a good care about you async / await calls 🙂