davidfowl / AspNetCoreDiagnosticScenarios

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

await Task.WhenAll(task1, task2); task1.Result; task2.Result #65

Closed worming004 closed 4 years ago

worming004 commented 4 years ago

Hello,

Very great list of best practices. It guides me and coworkers about multiple async await usage.

But there is 1 question where people disagree.

Regarding the following code

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
await Task.WhenAll(task1, task2);
var result1 = task1.Result; 
var result2 = task2.Result;

We agree that starting the 2 first tasks before waiting any result is good. But we disagree how to get the result.

By following best practices to get the value using await on each tasks is imo safer. But I do not have much arguments about it. I prefer the following version. It's more readable. But I miss some knowledge about performance and safety.

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
var result1 = await task1; 
var result2 = await task2;

What is your mind ? What are the best version regarding safety, performance, readability ?

jjavierdguezas commented 4 years ago

I've always had this doubt Regardless of the use of Result, the first of your codes could execute faster since Task.WhenAll could execute both Tasks in parallel. I wonder if in this scenario the first code will be considered good practice 🤔 Maybe a modified version of the second one

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
await Task.WhenAll(task1, task2);
var result1 = await task1; 
var result2 = await task2;

It would be as fast as first due to Task.WhenAll and it is also avoiding Result (awaiting for the completed tasks should be immediate, but a little weird)

davidfowl commented 4 years ago

Accessing .Result would be faster after the task has completed. awaiting the task is more idiomatic but results in a bigger state machine with more states (one for each await point in the method). Look at the codegen here:

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEAMACFBGArAbgFgAoEgNwgQykTIEsBjODAXgwDs4B3DAZVsZwAFAEoipYhSoxoAa2ytqApgDoA4nBgBBKAE92DIQCIAFjBgAHEAHprXeyuB12AcxUMA9mCNjylDDJQsgBMijQI9Koa2noGxmaWNnYOLh4eLgA2cO5ePuIoAJxYAGwqAOomcOxaGRlCgfIANAFywb6S/ghwUACuGTAKbA3YKgBK3X0w+Bh+VF29/aFDrWMT/eIkKKH8EYIzxADeJBgnWADMJQA8OGgAfBjROvqGNxg9CBkirPcopQBiCC84wWMCE70+4gAvkA=

VS

https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEAMACFBGArAbgFgAoEgNwgQykTIEsBjODAXgwDs4B3DAZVsZwAFAEoipYhSoxoAa2ytqApgDoA4nBgBBKAE92DIQCIAFjBgAHEAHprXeyuB12AcxUMA9mCNjylDDJQsgBMijQI9Koa2noGxmaWNnYOLh4eLgA2cO5ePuIoAJxYAGwqAOomcOxaGRlCgfIANAFywb6S/ghwUACuGTAKbIUtQdj4GH5UXb39oUNFDcHiJCih/BGCE8QA3iQY+1gAzCUAPDhoAHwY0Tr6hucYPQgZIqxXKKUAYgheAErdfRgQieL3EAF8gA=

jjavierdguezas commented 4 years ago

So @davidfowl, would would recommend the first version?

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
await Task.WhenAll(task1, task2);
var result1 = task1.Result; 
var result2 = task2.Result;
alefranz commented 4 years ago

I think another aspect to consider in picking one solution over the other is how do you want to handle exceptions, in particular on how you want to handle the scenario when only some tasks are failing.

worming004 commented 4 years ago

Based on feedbacks, conclusion is that first version

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
await Task.WhenAll(task1, task2);
var result1 = task1.Result; 
var result2 = task2.Result;

have quite more performance than second version.

But second version

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
var result1 = await task1; 
var result2 = await task2;

is more idiomatic.

Thanks for precision, it's enough to restart debate with my coworkers.

worming004 commented 4 years ago

I've got enough feedback for me. Ticket can be close IMO. But I see more concern raised by other people.

I let other people close this one, as some questions are pending. If there is no more activities several days, I'll close myself.

Thanks

andleer commented 4 years ago

A third version was introduced by @jjavierdguezas mid conversation and it isn't completely clear to me who is talking about which...

var task1 = service.GetAsync1();
var task2 = service.GetAsync2();
await Task.WhenAll(task1, task2);
var result1 = await task1; 
var result2 = await task2;

Does the await Task.WhenAll(task1, task2); improve or harm performance? @davidfowl uses this in his comparison. What if that line is omitted? Does it simplify the state machine denoted by @davidfowl ?

worming004 commented 4 years ago

Wihout any update, I close this issue. Thanks everyone

davidfowl commented 4 years ago

@andleer that's even more awaits 😄 so it's not as efficient.

andleer commented 4 years ago

@davidfowl agree that it is even more awaits but you used that scenario in your 2nd sharplap.io / codegen example. I realize it was most likely unintentional but I wonder if the inclusion impacts your comment earlier in this thread. Either way, I will take idiomatic over minimal performance differences. Thanks for all the contributions here!