Using Async suffix on async methods

[damianh]

With a nod to our friends over at Particular Software the team here fully agrees with their Reason for No Async Suffix naming convention. As such we are looking to adopt this across all of Duende's code base.

As this is a technically a breaking change SemVer wise we plan to role this adjustment out gradually.

• All new code will not have the Async suffix for asynchronous methods.
• Existing methods with Async suffix will be renamed at the appropriate time aligned with a major version of the given library. (We will not be shipping major versions just to make this adjustment).

We value consistency but we have to make a trade-off against disruption. As this is gradual, we expect there to be mixed conventions in a given library during the transition period.

[stefannikolei]

I can understand your standing point.

But from a user perspective it is easier when the methods end with Async.

Having async in the name makes it easier to spot mistakes in CodeReviews. So it is easy to identify while reading the code.

While writing the compiler helps. But it is a learned thing from asp.net core and bcl that you have async in the name. That makes it consistent.

[damianh]

Thanks for the feedback.

The reason MS adopted adding the Async suffix is because they were adding async apis along side there synchronous versions (and c# doesn't facilitate overloading on return types). Thus they had to use a naming convention. Also it collides with their guidance of Async suffix in their event based asynchronous patter. Similar to what is mentioned in the linked Particular Software docs, we have no such requirement to support both synchronous and asynchronous versions of an API.

[gingters]

Async methods should have the Async suffix.
We had a case where a customer called us to help him debug a nasty problem. They didn't await an async method because the suffix was not there on that specific method. And of course we did not spot the mistake at once, as this was not obvious. That was an extremely expensive week(!) of debugging that thing with our customer, what we could have spotted at an eyes blink.

And no, the compiler did not help: In the real world, customer projects, especially if grown organically over years up to decades, tend to have hundreds if not thousands of compiler warnings. That one won't pop out.

Changing that in your codebase will introduce massive potential problems with your customers.
.NET is strongly opinionated, and so are the default coding guidelines. And good citizens in that ecosystem stick to the rules, even if that means putting an redundant Async suffix on each and every method. Consistency within the ecosystem is what makes .NET so strong compared to other environments. Please do not break that.

[damianh]

Thanks for the feedback, I have some challenges :)

• Adding an Async suffix won't magically make a developer not await it.
• The compiler does actually help. There are more examples in the Particular Software post.
• The ecosystem is not actually consistent in this regard. There are lots of libraries that don't follow this when their primany function is async based. NServiceBus, Mediatr, Masstransit to name a few. (I'd name drop my own but obviously I have a bias here 😄 )

Changing that in your codebase will introduce massive potential problems with your customers.

We are incredibly aware of this. The API stability is a hallmark of IdentityServer over the years. We won't be arbitrarily changing method signatures "just because".

[Tornhoof]

Personal opinion, keep it, I like it, the reasons are already stated above. Especially @gingters point about the coding guidelines.

If you change it, your migration path should be tool supported, e.g. provide a roslyn codefix to change the existing names.
You can't simply search & replace every MyNameAsync(...) to MyName(...), because it likely will collide with other code in the customer's code base, so this needs to be done manually by each and every dev on update of your library to the new major version.

[damianh]

Thanks for the suggestion @Tornhoof. I really like this idea if we go about changing existing signatures.

[SamuelBerger]

One day Microsoft might "come to its senses" 😇 and follow Java's virtual threads (project Loom). Then we will all be happy to have the async methods nicely separated. Keeping it saves energy and is more future proof at the same time.

[kaysho]

Better to keep the Async suffix

[RacerDelux]

I would prefer the async method. It just makes it clear that it is awaitable. I would second the points given above as well.

[No1e]

Please keep the Async suffix.

[frankbuckley]

Please don't. Why make your codebase less intuitive by not sticking to the common conventions used in most .NET codebases? Why add unnecessary friction?

By convention, methods that return commonly awaitable types (for example, Task, Task, ValueTask, ValueTask) should have names that end with "Async". Methods that start an asynchronous operation but do not return an awaitable type should not have names that end with "Async", but may start with "Begin", "Start", or some other verb to suggest this method does not return or throw the result of the operation

https://learn.microsoft.com/en-us/dotnet/csharp/asynchronous-programming/task-asynchronous-programming-model#BKMK_NamingConvention

[zybroxz]

I think this is a bad decision and breaks years of well defined naming conventions. How hard can it be to just keep using the suffix? Who cares if NServicebus have decided to drop the suffix, 99% of .net developers will continue to use it

[damianh]

Thnks for the feedback. We won't be making breaking changes just just for a naming convention. This post is more concerned with new code/libraries we'll publish. We'll take the feedback into consideration.

99% of .net developers

I'm a 1 percenter hah

[b1tzer0]

Not to mention that tools like SonarQube will complain about it as well.

[damianh]

SonarQube will complain that a external library's API doesn't have an Async suffix?? Got a link to relevant docs for that?

[kyle-sexton]

Its not the end of the world if you dropped it, but I think most of the .NET community expects Async suffixed methods at this point.

[rahul7720]

I think keeping the Async suffix as it is a good idea:

1. It follows the common .NET naming convention.
2. It helps clearly tell the difference between sync and async methods.
3. Some classes may have both versions, like:

   var profile = GetUserProfile();
   var profileAsync = await GetUserProfileAsync();

Without the suffix, it can be confusing to know which one you're calling. Keeping Async makes the code easier to read and maintain.

[damianh]

In comparison to the .NET APIs, we have no requirement to support both synchronous and asynchronous versions of an API. We will never have a situation where we'd have a GetUserProfile() and GetUserProfileAsync(). Therefore the need to add the Async suffix does not technically apply (because one can't overload on return types in C#).

[RacerDelux]

In comparison to the .NET APIs, we have no requirement to support both synchronous and asynchronous versions of an API. We will never have a situation where we'd have a GetUserProfile() and GetUserProfileAsync(). Therefore the need to add the Async suffix does not technically apply (because one can't overload on return types in C#).

Counter point.
Sometimes I DO want a blocking call and purposefully use a non-async method.
While I do agree that within IdentityServer both don't exist, following the convention makes it fit better into our code base.
Also, changing it would be a MASSIVE breaking change for us, as we have a custom implementation. Sure it is a refactor, but it would still require a total retest of the application.

[damianh]

We're not going to make a breaking change just for this. We won't foist such on our customers.

[twenzel]

I would prefer dropping the Async suffix. I guess Microsoft introduced this suffix at a time when they were forced to provide both, synchron and asynchron variants of their methods.
Nowadays it's not required anymore. And the compiler is doing a great job identifying all possible mistakes.

[damianh]

Yes that's the sole reason they adopted the convention. We don't have such legacy backwards compatibility requirements.

[awaescher]

My two cents are, that the async suffix does not provide any meaningful insights but introduces more noise instead. We have clever compilers, tools like SonarQube and many others that can hint to unawaited tasks. Also, I'd recommend declaring these warnings as errors so that they must be fixed.
There are a dozens of things that makes a method signature, why chosing only async to be included in the method name?

I would love to drop the suffix in my projects but in OllamaSharp people where complaining about the lack of the suffix. After I talked to the folks at Microsoft while building the bridge to Microsoft.Extensions.AI, they recommended to use the suffix as well, so I finally added it. Still no fan.

[CrossBound]

Not everyone out there uses fancy compiler tools like SonarQube

[smintrh78]

My opinion: 100 percent: please keep the async suffix.

[CrossBound]

Please keep the Async suffixes. This is standard convention and I don't want to see this removed. This is standard practice for me when I'm designing code, and it makes it easy to see at a glance what the expectation is for the method without having to inspect the response.

[vanbukin]

If you want to adopt something good by making breaking changes, then it's better to adopt the normal transmission of CancellationToken through method parameters, because the current extraction of CancellationToken inside the methods of the IdentityServer codebase is comparable to repairing a car engine through an exhaust pipe. And leave the async suffix alone, this is a common practice in .NET is understandable to absolutely everyone, and all these lengthy discussions about whether to write this suffix or not come from groups that do not want to be like everyone else. In fact, everything is fine. The NET ecosystem looks at how Microsoft does, taking the dotnet/runtime and dotnet/aspnetcore codebase as a reference. And if a suffix is used there (and it is used there), then everyone will use it. And there's nothing you can do about it. If you want to make breaking changes, then let them bring real benefits.

[CrossBound]

Completely agree that cancellation token should be optional parameter in all async methods.

[damianh]

Agreed. This is definitely on our radar.

[RacerDelux]

100% agree on the cancellation token part too

[josephdecock]

If you have an opinion on CancellationTokens, please share it with us in this new thread: https://github.com/orgs/DuendeSoftware/discussions/285. Thanks!