Facepunch / sbox-issues

173 stars 11 forks source link

Async Methods Naming #5832

Open Andrei15193 opened 1 month ago

Andrei15193 commented 1 month ago

What it is?

The API contains a methods that are asynchronous or return a Task (or any awaitable) which are not suffixed with Async.

The general convention is to suffix methods that may not perform the operation synchronously with Async as this clearly indicates that it should be awaited, it increases code readability as often enough synchronous and asynchronous calls get mixed in the same method.

While some IDEs provide warnings for Task returning methods that are not awaited, it is not always the case. This is also relying too much on the IDE to tell us when we forgot to await something and when it doesn't work we have to dig into the code and find where we missed some awaits.

The presumption that it is obvious has nasty side-effects when an obvious async method is not so obviously async.

The C# compiler does not always give warnings when we call a Task returning method.

public void HiddenFireAndForget()
{
    Task.Delay(1000); // no warning
}

public Task HiddenFireAndForget()
{
    Task.Delay(1000); // no warning
    return Task.CompletedTask;
}

// Only when we use the async keyword we start getting warnings
public static async Task HiddenFireAndForget()
{
    Task.Delay(1000); // generates warning
}

References

What should it be?

Have all Task returning, or any awaitable type for that matter, methods be suffixed with Async to make it obvious to the reader that they should await such methods if they are calling them directly.

The exceptions should be treated on a case-by-case basis, similar to Task.Delay there's no need to add Async at the end in this case as you would expect to get a Task and know you should await it.

This will keep consistent with dotnet so calls to either to dotnet API or Sbox API would not seem different.

Users will not have to check the return type of each method to know whether their code should also be async or not.

Having this as the standard can help library developers follow the same approach, the Sbox API setting the example for extensions, thus making it easier for library users to also know which methods are async and which are not.

Andrei15193 commented 1 month ago

The same issue is raised with #2380, this one is to cover the entire API. The argumentation is the same, it helps with readability as IDEs and compilers do not always issue warnings when an async method is not awaited or handled.

If you write a sync method and at some point you end up adding an async method without knowing (you did not check the return type which can be a plain Task), you unintentionally add a fire and forget.

garrynewman commented 1 month ago

I really don't agree with this naming scheme. It's messy. The only time I would advocate adding Async is when you have another synchronous method of the same name.

Andrei15193 commented 1 month ago

I'm more pro-suffix hehe

I've had more than one occasion where I should have awaited an async method but didn't simply because I didn't expect it to be and it wasn't obvious to me either. Other people I have worked with have run into similar issues and had to go through their entire code to check if there are any missing awaits and then dealing with its ripple effect.

It's a tedious process if you end up forgetting an await and that's the main reason I prefer the suffix. It tells me right off the bat that the method may not complete when it returns which can lead to other problems such as uncaught exceptions, deadlocking by accessing .Result or calling .Wait() on the returned task, unexpected state changes on the object whose method I called etc.

On the other hand, from my perspective, I see the mixture of async and non-async (standard?) methods being messy as well because they have a similar signature and sometimes I await a call and other times I do not. To me, there's a fundamental difference between the two and without the suffix I only have the return type to distinguish between them.

As an alternative example, most JavaScript libraries that expose async methods do not follow this naming scheme and I'm always left wondering if I should use await or not. What we can do in JS, but not in C# is that we can await even non-promise objects or values and you basically get the value directly.

Always using await when I'm not sure if I should, but I don't want to risk it because of side-effects, just shows how ambiguous it gets when we don't have the suffix telling me plainly that a method should be awaited.

These are my experiences with and without the suffix and why I prefer to have it, I'm also biased because I started playing with async stuff when it came out in dotnet and everything was suffixed and the guidelines from Microsoft also indicate this.

MD485 commented 1 month ago

I agree with Garry and Matt on this one, take WebSocket, is anyone going through the methods and saying to themselves: "Connect() is async! How could I have known! Send() too? What an unexpected turn of events!"

While this isn't as true for every line of code facepunch has written, like for example I was caught out a little when some of the functions in the SDF library were async, it's true often enough that it doesn't feel like it matters.

Using a class I've never used before I always check the method signatures for return types and parameters, if those aren't in every method name, why should async be the exception? If someone is editing async code someone else wrote that they don't have a good understanding of, will the added keyword in the method name be enough to prevent bugs?

Plus the longer a method name is the more likely I won't fully read it, so having asyncs everywhere would lower readability for me.

Task I feel is also more of a counter point than a point in your favour, someone who hasn't used Task before may assume that Task.Delay() does the same thing as Thread.Sleep(), reading what the class actually does gives you the insight that it doesn't. It's not like Task doesn't have any synchronous methods, but it only uses async when there's duplicate method names. I feel like you don't mind it simply because you're used to it. 😅

Andrei15193 commented 1 month ago

To be honest, I wouldn't be surprised if the Connect() method on a WebSocket is not async. I would rather assume it's sync than the other way around because of the other SDKs and dotnet itself.

I don't always check all the methods of a type, it depends on how many it has. LINQ being an example here, you get a lot of extension methods for enumerables, I haven't checked all of them in one go because there are so many. ADO.NET has quite a handful of types of accessing data, when I was using it I didn't check all methods on any one type, but rather did it incrementally. Even with the Sbox SDK, I don't always check all the members of a type unless it's only a handful, I may scan the interface to get a general idea.

On the other hand, having very long method names is rather an indication that the name itself is not good enough and needs more work. If just adding the Async suffix makes it too long then there is probably better wording for it.

About Task.Delay() being a counter point, that would be the exception that enforces the rule for me. If you are using methods of Task that generate a Task but you do not look into it, then what are we doing? Confusing Task.Delay() with Thread.Sleep() is not that big of a deal, you run it and realize it doesn't do what you expect.

When you have a task and want to chain a callback using .ContinueWith, it is no surprise to me that I get something with delayed execution because the task whose method I call already represents something with delayed execution.

It's not just that I'm used to it, I had to work with both cases and I've had a better overall experience when async methods had to suffix. It's far easier to spot and the side-effects when it got past code review were nasty.

I see both sides of the argument, and I also smile a bit when I see the documentation pointing out when a method is async as a "hey, watch out! this is not like the others!". It's kind of telling that we implicitly accept them as being different, but not enough to make it more obvious except through written documentation.