davidfowl / AspNetCoreDiagnosticScenarios

This repository has examples of broken patterns in ASP.NET Core applications
7.69k stars 740 forks source link

[Question]: Is Task.Run useful in this situation #50

Closed NicolasDorier closed 5 years ago

NicolasDorier commented 5 years ago

In this situation, does Task.Run is useful at all? Knowing that FireAndForgetAsync is not too important (it might raise exception, but we don't really care)

public async Task<IActionResult> DoSomething()
{
      await repo.SaveDataAsync();
      _ = Task.Run(FireAndForgetAsync);
}

public static async Task FireAndForgetAsync()
{
     await repo.SaveOtherDataAsync();
}

Or

public async Task<IActionResult> DoSomething()
{
      await repo.SaveDataAsync();
      _ = FireAndForgetAsync();
}

public static async Task FireAndForgetAsync()
{
     await repo.SaveOtherDataAsync();
}

You seems to use Task.Run everywhere in examples, but is it really needed?

davidfowl commented 5 years ago

I'll explain the difference and let you decide. If you don't Task.Run, the fire and forget method will run synchronously until the first await happens (and that await actually has to go async). What that means is that you can end up executing everything inline if it completes fast enough. It may or may not matter depending on your expectations though.

In essense, Task.Run means defer all work so nothing happens on the calling thread (schedule it to run later), while invoking the method directly will start it on the calling thread (which can have side effects)

NicolasDorier commented 5 years ago

Thanks a lot. It is what I thought, but I thought that maybe your use of Task.Run had some other non obvious effect on the synchronization context or on the way uncaught exception are treated.

slang25 commented 5 years ago

I started answering this, but can't beat @davidfowl explanation. I'll add a concrete scenario where I would prefer not to use Task.Run, and that's when you are calling a private method or local function you know isn't doing any heavy lifting (computation or blocking) before yielding execution, and will very immediately await an incomplete task, here is an example:

https://github.com/slang25/Channels.Extensions/blob/3062bfcb22a98a4803d1da85de1b8f2176e61906/Channels.Extensions/Class1.cs#L51

Now, there's a chance that all of my awaits are against completed Tasks, then I will perhaps unwillingly run more synchronous work than I bargained for, and that's another reason why it's hard to have a general rule.

NicolasDorier commented 5 years ago

Thanks a lot, it makes total sense.

davidfowl commented 5 years ago

@slang25 you can delete this now https://github.com/slang25/Channels.Extensions/blob/3062bfcb22a98a4803d1da85de1b8f2176e61906/Channels.Extensions/Class1.cs#L13. It's been added https://github.com/dotnet/corefx/blob/355917229c2e5a35b8ea7f14d5d6de2455c5dc1b/src/System.Threading.Channels/src/System/Threading/Channels/ChannelReader.netcoreapp.cs#L18

slang25 commented 5 years ago

Oh nice! Channels and async streams play really nicely together 😀