Ordering. Fixed async/await misuses

[Marusyk]

Fixed some async/await misuses. Please review. Thank you in advance

[mvelosop]

Hi @Marusyk, thanks for the PR.

Will start reviewing the changes, but want to comment on something right away.

Taking a glimpse at the changes, I realized there are several that involve removing the async/await keywords and just until last Wednesday I would've thought they were all right, actually, I had started doing that kind on changes to my codebase from a video I watched on Channel 9.

But then I looked at src/Services/Ordering/Ordering.API/Application/Commands/IdentifiedCommandHandler.cs and started digging to find a way to remove the async/await in that case and I found this article: http://blog.stephencleary.com/2016/12/eliding-async-await.html from Stephen Cleary (Mr. Async).

So from now on I'll be following his recommended guidelines (and rolling back my changes).

Thanks again!

[Marusyk]

Hello @mvelosop

Thank you for suggestions.
Do you mean I should revert that changes to follow this rule

1. Do not elide by default. Use the async and await for natural, easy-to-read code.

The thing is, the state machine created. When a method only awaits one operation and does so at the end of the method, it's usually not worth it. Instead, we can directly return the task, as shown in the first method, and avoid the cost of the async state machine altogether.

It seems we can't remove the async/await here https://github.com/dotnet-architecture/eShopOnContainers/pull/568/files#diff-fe2c237e4c0d12551a80125619acb142

[mvelosop]

Hi @Marusyk, I was actually doing the same as you until a week ago, but then, upon reading http://blog.stephencleary.com/2016/12/eliding-async-await.html from an expert in async and concurrency, I realized that eliding the state machine could make debugging async code much harder (if that's even possible!).

I suggest you take a deep look at the above post.

[Marusyk]

Hi @mvelosop,

Ok, as I said, thank you for suggestions.
I think there are not only one expert in async and concurrency and I also can find some articles from other "experts". But as I see this discussion doesn't make sense because "an expert in async and concurrency" write http://blog.stephencleary.com/2016/12/eliding-async-await.html

If you sure then close thie PR.

Kind regards,
Marusyk

P.S. Please do not share the link http://blog.stephencleary.com/2016/12/eliding-async-await.html again. I've got it (it was 6 times)

[mvelosop]

Hi @Marusyk, please, don't get me wrong, I also watched another expert on channel 9 suggesting to return Task whenever you have a "return await" and that's what I had been doing until just now.

I'm no expert in async, but the article explained three situations with enough detail to make sense out of the "new" recommendations.

Regarding your PRs there are other changes, like adding cancellation tokens, fixing names and others, so there's no need to close them.

They will be kept on hold, however, as @CESARDELATORRE pointed out, because of some mayor changes coming, although, as far as I could check, there are no conflicts with your changes.

Regards,
Miguel.

P.S. I'm sorry about the link!

[mvelosop]

Hi @Marusyk,

@CESARDELATORRE did check on this issue with the .NET Team (Stephen Toub, Immo Landwerth and Mads Torgersen) and this was their response:

“Recommend using async/await by default, and only removing them when doing optimizations because of known perf issues (or you’re implementing libraries you expect to be used on super hot paths). The one exception to this is for public methods that need to do argument validation, in which case just as with enumerables, you generally want to do that validation in non-async methods so that the ArgumentExceptions are propagated out of the method rather than being captured into the returned task.”

If you want to go ahead with the suggested changes, I'd would like to ask you a favor, for trying a procedure to resync the repos and solve the situation like one you have in your repo, that dev is both +3 and -31.

This happened because of an issue during the recent merge of the bff branch.

I can't use my fork for this because while testing different ways to solve this issue my fork lost the state I need to test this last version.

Please let me know if it's ok with you and want to help solving the branching issue.

If so, I would send you the guide privately for testing, before publishing it here.

Thanks.