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! 🙂