Create rule S6966: Awaitable method should be used #3854
Conversation
martin-strecker-sonarsource
changed the title
martin-strecker-sonarsource
requested review from
gregory-paidis-sonarsource
and removed request for
antonioaversa
martin-strecker-sonarsource
assigned
gregory-paidis-sonarsource and unassigned
antonioaversa
gregory-paidis-sonarsource
left a comment
gregory-paidis-sonarsource
left a comment
There was a problem hiding this comment.
I did not look at the code snippets yet.
I release the review so that you can take a look.
Excellent write-up!
I like that it's very to-the-point, while also describing the problems of blocking.
Nice job!
Most of my comments are phrasing suggestions or more documentation links.
(looking at code now)
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "async-await" |
There was a problem hiding this comment.
What do you think about performance as well?
There was a problem hiding this comment.
I'm not sure. This is about freeing up a resource (thread), that is not needed until the awaited task is finished. That may or may not be beneficial performance-wise.
For me, this rule is more about correctness: If you are async already, then be as async as possible and slice your method at every opportunity.
What do you think?
There was a problem hiding this comment.
I get your point, but at the end of the day, what happens if you are inefficient about freeing resources?
If your application does not need to scale, it really doesn't matter.
The only time it matters is if you want to use all the resources available in an optimal way.
And if you fail at that, it means you are serving < MAX clients, or something similar.
I would ask Denis as well, as it also has to do with how we want to promote it.
gregory-paidis-sonarsource
left a comment
gregory-paidis-sonarsource
left a comment
There was a problem hiding this comment.
Very good tests.
Left a couple of suggestions and questions.
|
|
gregory-paidis-sonarsource
left a comment
gregory-paidis-sonarsource
left a comment
There was a problem hiding this comment.
LGTM!
One comment remaining, please consider asking Denis' opinion.
4 participants
You can preview this rule here (updated a few minutes after each push).
Review
A dedicated reviewer checked the rule description successfully for: