Identityserver4: Get rid of all `Async` ?

[leastprivilege]

https://blogs.rocksolidknowledge.com/2015/12/02/lets-stop-the-async-suffix-bullshit/

[brockallen]

Too soon, prolly

[klanglie]

Probably better to do now while in Beta. I just renamed all my methods, removing 'Async.'

[leastprivilege]

I also think it is now or never.

[brockallen]

Sure, but is there real precedent elsewhere?

[leastprivilege]

SignalR as it turns out...

[brockallen]

We should ask them if that was the right thing to have done in retrospect. // @davidfowl

[tibitoth]

_Confession Bear_ I personally like the Async suffix.

[leastprivilege]

e.g. here

https://msdn.microsoft.com/en-us/library/dn433692(v=vs.118).aspx

[filipw]

Async is absolutely hungarian notation, get rid of it.
I never use it personally unless you want to overload a sync method (since you can't overload by return type)

[imranbaloch]

Another suggestion keep it just for back compatibility(with IdvSvr 3) and make it obsolete.

[brockallen]

Well, given that they know they're cool now, I guess we keep it: https://twitter.com/leastprivilege/status/753472703387099136

[DerAlbertCom]

Getting rid of *Async makes not really sense. I think a convention is needed to know which methods are awaitable. And in my Code ihave Sync and Async Methods. Because normale local computation does not need to be Async. And not knowing which is awaitable or not is bad for productivity.

Also it is not even less typing on using sync method, because most times i complete method names with Tab i dont write the complete name there.

[michael-x]

Worst thing, when you are calling a method and you are not aware that it is Async and therefore you forgot to add the await and half of the time it works, because the Task is short, but half the time it doesn't and it's right in the middle of other async logic and you try to debug it, but you can't reproduce it. On GetResultAsync you could say GetResult, because when trying to use the result you'd notice that you have to await it, but for something like DoSomethingAsync, naming it DoSomething instead just asks for problems.

[leastprivilege]

I am with @filipw - but I agree that it is confusing for many developers. unfortunately.

[bacathey]

My team dumped the suffix as we transitioned more and more of our APIs to be asynchronous and realized the fact that, in a traditional data-oriented web app, there will be more async than sync. After a while the suffix stops adding any value. The Task<> return type should be a very strong hint that you'll need to await, or at least double check for an awaitable. Dump the suffix and assume your clients are smart enough to figure it out. They'll at least figure it out on their first unit test that uses a mock of you code.

[michael-x]

@bacathey how would a unit test show, whether you have awaited the returned Task of a mocked Task DoSomething() method? You can certainly check, whether DoSomething has been called, but can you also check, whether the Task has been awaited? (Or will you even remember to test that, if the method is simply called DoSomething?)

[scottbrady91]

It's a shame to see the biggest argument against removing async is that people won't know if a method returns a Task/is awaitable. Does no one look at the return types?

[leastprivilege]

@scottbrady91 I totally agree

[michael-x]

There are certainly methods where you would expect them to return something (like Get..., Is...) and there are methods where you may or may not expect them to return something (like Save, Do...). Even if they do return something, you may not be interested in what they return (e.g. if Save returns a bool that tells you whether it worked and you are almost always sure that it will). The point is, a good API is one that is easily and intuitively usable without having to worry about 'having to look at the return types'.
Btw. on a method IsActiveAsync (IProfileService) I would expect it to return a Task telling me whether the user is active in a particular context. Otherwise I would expect the method to be named ApplyIsActiveAsync or something similar. Having an extendable IsActiveAsyncContext is a good idea, not returning the expected is not ideal.

[michael-x]

Why do you want to make things harder for people than necessary?

[klanglie]

Some implementations might not choose to implement a method as Async even though the name suggests it. By removing the ‘Async’ suffix you might be removing confusion to those who choose not to implement it as such.

[michael-x]

I'm not sure I can follow the reasoning here, @PivotalAnimal. I don't think a caller of an API should have to worry about whether the API does something expensive on a database or a web service or reading large files, or whether it just returns Task.FromResult on a cached value. No matter how you implement the method, the method would still return Task or Task<T> and if it does you should await it if you want to make sure that whatever you are doing in the method is done before you do the next thing. A method could even go so far as to check for a cached value and, if that exists, return that synchronously through Task.FromResult, but if not, go to a database with some Async method.
Even those people wanting to implement the method synchronously would have to follow the Async pattern (returning a Task), so keeping the Async would remind them to do so. Otherwise they might just return null, breaking other things.

[mcquiggd]

Personally I cannot see the positives from doing this, so to me this seems an unnecessary course of action.

Even the advocate blog post quoted in the starting message gets confused:

If you have to support both a sync and async version of an API then maybe still use it – or rather, to encourage async interaction suffix the synchronous version with a Sync suffix as in NodeJS APIs.

It seems to be similar to the "JavaScript should not use semi-colons" arguement - it's purely down to personal taste, opens up a (small?) additional possibility for error, doesn't add any extra functionality, and there is no technical reason for doing it.

Edited: In the Twitter response from people in the SignalR team (and Auth0), they mention removing the Async suffix; it caused confusion, so they put it back / plan to add it back.

https://twitter.com/leastprivilege/status/753472703387099136

@brockallen I think that makes it pretty clear.

[leastprivilege]

OK - the consensus seems to be to stay with Async. thanks for your input.

[fourpastmidnight]

I know this is water under the bridge (or over the dam) but I'd also like to point out that in all the documentation I see regarding APIs and the use of async, it's a _guideline_ that Async be used as a suffix for asynchronous methods using the async/await pattern. (And as a _guideline_, it doesn't need to be adhered to as if it were law.)

I don't disagree with anyone here that the suffix is a form of Hungarian notation. I also would rather not see it and use async by default and suffix methods with Sync for synchronous versions as well. And, usually, where conventions don't make sense, I feel free to do otherwise. _But_, a framework or library are probably not ideal places to lightly brush off the convention. I just want to caution that due to the conventions in use for the larger .NET Framework, and 3rd party libraries which are meant to be used almost as an "extension" of the framework (such as Identity Server), that a change such as this shouldn't be made lightly.

I wholeheartedly hope that at some point we can drop all the Hungarian notation non-sense (even the I prefix or interfaces, for example) throughout the wider .NET Framework; but I admit, that's very unlikely to happen--because compatibility and interoperability and ensuing readability and understanding of the code making use of (possibly) disparate conventions.

Perhaps with libraries such as IdentityServer, it would not be a bad thing to "buck" convention, if only to start the conversation (or finish it...)--especially if other like-minded libraries follow suit--but I think it should not be taken lightly all based off of one blog article. Again, I do fundamentally agree with the author of the blog article.

Anyway, I appreciate all the comments made on this issue and enjoyed reading all the feedback. I'm equally glad that the Async convention is being adhered to--for now.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.