Improve asynchronous calls

[janpieterz]

Following advise we always follow in libraries:. There's probably a couple more places where there's an explicit async/await where it can be avoided, as for example:
public async Task Send(CommandPing command) => await Send(command.AsBaseCommand());
can become
public Task Send(CommandPing command) => Send(command.AsBaseCommand());

• https://blog.stephencleary.com/2012/02/async-and-await.html
• https://medium.com/bynder-tech/c-why-you-should-use-configureawait-false-in-your-library-code-d7837dce3d7f
• https://stackoverflow.com/questions/13489065/best-practice-to-call-configureawait-for-all-server-side-code
• https://msdn.microsoft.com/en-us/magazine/gg598924.aspx

[blankensteiner]

Hi Jan-Pieter

Thanks for the PR! It's greatly appreciated so keep 'em coming! :-D

Let have a talk about this and break that talk into two parts.

1. Directly returning vs async/await
2. Using ConfigureAwait(false)

Directly returning vs async/await

Before DotPulsar moved to GitHub, for a long time I was returning directly whenever I could. The reason why I now consistently use async/await is because of David Fowler's guide: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task
Stephen Cleary also says to default to async/await: https://blog.stephencleary.com/2016/12/eliding-async-await.html

Using ConfigureAwait(false)

You may be right that we need to do this, however, I don't think the answer is so clear.
If we again look to David Fowler: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#configureawait
then we see that it's sadly left unanswered, even though there is an issue for it: davidfowl/AspNetCoreDiagnosticScenarios#31
Using ConfigureAwait(false) also has some disadvantages: https://github.com/Microsoft/vs-threading/blob/master/doc/cookbook_vs.md#justification
And finally, I don't quite know how to interpret what Stephen Cleary writes here: https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html
Does that mean that if we don't ever block (call .Wait() or .Result, which we never do), then we are golden and can avoid spamming the code with ConfigureAwait(false)?

Looking forward to hearing your thoughts on this :-)

[janpieterz]

Thanks!
Regarding the direct returned async/awaits, I'll agree with David 😉
The ConfigureAwait story, I think the Stephen Cleary comments are important as this is a library, and ideally you'd not block behaviors for upstream users. They can still await your final task as ConfigureAwait(true) but if you don't do it, they never can make the choice the other way, hence his comment to recommend it for libraries.

For us, we always just do the (false) option and have an analyzer for it, ensuring that all code always uses at least one or the other.

[blankensteiner]

I wonder why Microsoft haven't just made a property for your csproj with what the default should be for your project and then automatically append ConfigureAwait(theValueFromCsProj) to all awaits when compiling...
Anyway, so we agree to async/await and ConfigureAwait(false).
I see you have added ConfigureAwait(false) to the Test project also, but since it's not a lib we can avoid it there right?

[janpieterz]

Agree, it's something the Fody project did, which I use sometimes (https://github.com/Fody/ConfigureAwait). It's an option.

The Test project can be removed yeah, as I'm having the analyzer on it was easier to just have them all done.

Let me know if you'd like me to:

• Implement Fody ConfigureAwait
• Remove Test project .ConfigureAwait(false) or leave

And I'll send a new PR

[blankensteiner]

Fody sounds awesome! Just one question. Fody is installed as a "build dependency", but ConfigureAwait.Fody is installed as a "runtime dependency". I would guess that both were "build dependencies"?

[blankensteiner]

I just did some testing on another project.
I added both as build dependencies:

<PackageReference Include="ConfigureAwait.Fody" Version="3.2.0">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Fody" Version="6.0.2">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

And then changed FodyWeavers.xml to:

<Weavers xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="FodyWeavers.xsd">
  <ConfigureAwait ContinueOnCapturedContext="false" />
</Weavers>

I can see with dotPeek that we get the ConfigureAwait(false) (on both Task and ValueTask) and Fody.ConfigureAwait is not added as a runtime dependency.

If you can confirm, then you can alter the PR to actually just adding Fody and then we can create a 0.7.1 :-)

[janpieterz]

@blankensteiner no clue how you got it to work, neither of the assemblies on my end get it. Any idea what's different?

[blankensteiner]

Damn, that was 1½ hours of my life that I will never get back. Damn you dotPeek!

Ok, I know what might have troubled you.

First, VS seems a bit weird, needing to restart (and/or clearing) sometimes before realizing that you have changed nuget-info on an existing package.

Second, dotPeek is acting up. The first problem is that it is caching decompiled code, so I have to restart dotPeek every time I change the dll. Second, I can't have the "same dll" open multiple time, meaning I have DotPulsar 0.7.0 in one folder and the DotPulsar 0.7.0 in another folder, because when I ask to see something in the second I get the cached view from the first. What a mess.
The third dotPeek problem was that only sometimes did I see ".ConfigureAwait(false);" in the decompiled code, but when I look at the IL, then it is actually there.
Could you try looking at the IL and confirm you also see it there?

One more thing, I found out we only need this when including ConfigureAwait.Fody (makes it a bit shorter):

<PackageReference Include="ConfigureAwait.Fody" Version="3.2.0" PrivateAssets="All" />
<PackageReference Include="Fody" Version="6.0.2">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

I'm btw ok when adding the xsd file to gitignore, but also ok with it being there. Up to you.

[janpieterz]

Right, that was odd. I ran into the same thing here. I did see it happen, but the ValueTasks didn't get the ConfigureAwait added on here. For now we can merge this at least, that'd be more something for Fody to pick up.