0

I was assigned to a new project, which was already running. I noted that people created all repository classes with async methods and they are called in the controllers with await, every time, so they are essentially working as synchronous code.

There is not possibility that they will be called asynchronously in future and the server, has a single SQL Server instance running.

It came from a kind of seed API project, the people who created it are no longer in the company and now no one knows exactly why they are async. But I think that in the current project scenario, it doesn't make sense to make them async at all, it's just adding unnecessary complexity to the system.

Does it make sense to make them async at all in such scenario?

rbasniak
  • 4,484
  • 11
  • 51
  • 100
  • One thing I would say is that if there's any possibility of anything needing to be async at any stage, you're better off making everything that way to begin with. I've worked on projects where something needed to be made async that wasn't already and the flow-on effects were a huge pain in the butt. – jmcilhinney Apr 11 '18 at 01:29
  • I don't understand your question. When you use the word *create* I think `new` and C# doesn't allow Constructors to be Async for a very good reason. Also whether or not the SQL is on the same box or not, `Async` will most likely give a performance gain when using async sql calls. – Erik Philips Apr 11 '18 at 01:36
  • @jmcilhinney I do agree with you, if there's an even remote possibility that something will be async in future, it's fine. But it's not the case here. – rbasniak Apr 11 '18 at 01:41
  • @ErikPhilips Sorry, what I meant is that all methods over the repository classes are async – rbasniak Apr 11 '18 at 01:42
  • "repository classes" - the Repository (anti-)pattern is one of the most abused concepts I see on SO - so you may be wasting time trying to suss this out when you shouldn't be doing this in the first place. – Dai Apr 11 '18 at 01:46
  • 1
    Awaiting an `async` method is not the same as simply calling a synchronous method because it still allows a UI to remain responsive while time-consuming work is performed. That's only really relevant to local applications though, not web applications. I don't claim to be an authority but, in your particular case, I'm not aware of an advantage to using `async`/`await`. – jmcilhinney Apr 11 '18 at 01:46
  • 1
    I think this question may help you get a better understanding of `async`/`await`: https://stackoverflow.com/questions/32482275/when-i-await-an-async-method-does-it-become-synchronous?rq=1 – Ehsan88 Apr 11 '18 at 01:57
  • @Dai, that seems to be cherry picking buzz words to make an unrelated claim. This question is independent of patterns. – ChiefTwoPencils Apr 11 '18 at 04:04
  • @Dai what would you recommend instead? I agree generic repositories are abused and definitely an antipattern. – Mardoxx Apr 11 '18 at 08:05

1 Answers1

6

You're confusing "synchronous" with "serial". Synchronous means "blocks a thread"; serial means "one at a time".

they are called in the controllers with await, every time, so they are essentially working as synchronous code.

No, they're invoked asynchronously and serially. The key here is that the ASP.NET request thread is not blocked while the data is being retrieved. This helps your web server scale.

the server has a single SQL Server instance running.

In my experience with that architecture, most of the time the single SQL server instance is going to act as your scalability bottleneck. So in this case, the async isn't going to help anything, since your web server can now scale, but it will still be limited by the DB backend (again, in most cases).

As for the question you didn't ask ("Does it make sense to make them all synchronous?"), I would say No. Why deliberately make the web server less scalable, even if you could? If there is the possibility you'll move to a scalable backend in the future (e.g., Azure SQL or a SQL Server cluster), then you'll want the web server scalability.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810