Is it a bad practice to use Singleton for DI in Asp.net rather than Scoped, Transient whenever possible?

Question

We had a conversation (with coworkers) about why we needed to use Singleton for Controller, Business Services, and Repositories. The reason for this, they claim, is because singleton ensures that code is thread-safe. According to what I read in The Gang of Four, we should use Singleton when:

• there must be exactly one instance of a class, and it must be accessible to clients from a well-known access point.
• when the sole instance should be extensible by subclassing, and clients should be able to use an extended instance without modifying their code.

So For me it better to use Scoped and Transient services whenever possible and not Singleton.

For example I am using those services as Scoped

services.AddScoped<INonTailorMadeCostComputerService, NonTailorMadeCostComputerService>();
services.AddScoped<ICostComputerService, CostComputerService>();

They want to transform like this

services.AddSingleton<INonTailorMadeCostComputerService, NonTailorMadeCostComputerService>();
services.AddSingleton<ICostComputerService, CostComputerService>();

Am I missing something about using Singleton, Scoped for DI? Is it a good practice to use Singleton and not Scoped/Transient whenever is possible?

Answer 1

Is it a bad practice to use X whenever possible?

Yes. It is bad practice to turn off your brain and lean on absolutes. It's far better to understand why.

So lets step back from that framework and try to find a more useful one.

The reason for this, they claim, is because singleton ensures that code is thread-safe.

Oh how wonderful it would be if we could just shove any arbitrary code into a framework and have it magically become thread safe. Sadly, that isn't what's happening here.

Transient objects are always different; a new instance is provided to every controller and every service.

Scoped objects are the same within a request, but different across different requests.

Singleton objects are the same for every object and every request.

Stack Overflow - AddTransient, AddScoped and AddSingleton Services Differences

What is happening is one copy of the object is being shared. Which actually sounds like it's less thread safe. But that also misses the mark.

Making a class thread safe is a lot more work than managing how it's created and how it's shared. There's also if it's mutable. There's if it's methods can be called concurrently.

So no. "singleton ensures that code is thread-safe" is a dangerous oversimplification.

So For me it better to use Scoped and Transient services whenever possible and not Singelton.

Don't know you or your needs but this is also sounds like a dangerous oversimplification. I'd feel better if you were saying things like: I have yet to find a need to ensure that every request gets the same copy of this object.

It's worth considering if you even need the code to be thread safe. Avoid exposing the code to multiple threads and you won't care if it's thread safe.

It's also worth understanding that what you're using here isn't the GoF's singleton pattern. You're using a library that named one of it's methods singleton. So be careful about applying too much of what the GoF wrote about the singleton pattern to this. They were looking at different code. Oh sure, the GoF ideas are just as valid as they ever were. But when you popularize a name for something it goes off and lives a life of its own.

Answer 2

Firstly, you are confusing singletons provided by a DI container with the "Gang of Four" singleton pattern. The latter is a glorified global variable and thus a complete anti-pattern and should never be used. Singletons provided by a DI container are almost totally unrelated. The only similarity being that you only have one of them in both cases, thus the common name "singleton".

Secondly, your co-workers are confused over thread safety. If I share a singleton that has state between threads, type safety is harder than giving each thread its own instance. So using singletons does not improve thread safety.

As to whether you should use singleton, scoped or transient for a type registration in the container depends entirely on the how the type is to be used:

1. If you only ever want one instance for the entire duration of the lifetime of your web application (ie until the service is restarted), then use a singleton. Things like access to configuration details will fit this.
2. If you want an instance that lasts for the duration of a user request (that eg might hold details of the user), then use scoped.
3. If you want a new instance every time the container is asked to provide one (a resource access request that needs disposing quickly for example), then use transient.

If the particular type works with registering it as a singleton, then do so. As the container will only ever create one instance, this reduces the work that the garbage collector has to do. At the other end of the scale, transients put the biggest pressure on the heap, so should only be used when necessary. And scoped sits neatly in the middle.

Answer 3

Before you change whether a service is Transient, Scoped, or Singleton, do make sure you are not introducing bugs. When something is designed to be thrown away quickly, there might be assumptions that the code was built around. If you put that code in a new context where it wasn't designed to run you can pick up undefined behavior due to the new context.

---

The advantage of Transient and Scoped is that the instances are not shared across threads. You have instances that only persist for short periods of time, which prevents those instances from requiring locks. The advantages of this approach include:

• Lockless design which prevents deadlocks or just lock contention
• Only keeping objects in memory as long as they are needed
• Reduced likelihood of memory leaks because the transient parent is destroyed as well.

The issue with Singleton objects is that you have to be very careful if you have persistent state. They are a common source of memory leaks due to a reference to data somewhere that won't be fully released. If you mutate persistent state, you have to lock your access to prevent race conditions. That increases the likelihood of lock contention or even deadlock--both of which are serious performance issues.

Answer 4

singelton ensures that code is thread-safe.

This is completely wrong. However, a singleton would be necessary to enforce thread safety for a shared resource, e.g. if you needed threads to synchronize themselves using a shared semaphore.

the sole instance should be extensible by subclassing, and clients should be able to use an extended instance without modifying their code.

These are good reasons to use a singleton over a static class. These days you should pretty much never be using static classes except for extension methods or possibly pure functions.

So For me it better to use Scoped and Transient services whenever possible and not Singelton.

A singleton is a good choice when

• The class is either stateless or immutable
• The class is stateful but the state can be shared safely and it makes sense to do so
• The class semantically represents something of which there is only one, e.g. the application's configuration, or the O/S environment
• An instance of the class is expensive to construct, so you only want to do it once
• The class's dependencies are all singletons

Answer 5

To put it mildly, what these engineers are proposing, is pure cancer.

In an effort to make the data repository and business services thread-safe by making every one of them singletons, the engineers have actually created vulnerabilities where threading problems and side effects can be easily introduced into the application.

Using addtransient avoids side effects and news up a separate copy with each request of the service, but using AddSingleton the DI framework creates a single instance of the service and shares it across all requests. This means that each HTTP request that comes in will use the same instance of the service, and any data stored in that instance will be accessible to all requests, thus creating thread safety issues where otherwise there would have been none including the potential to leak data across different user contexts adding security concerns.

Using this approach means every function, library, and service utilized throughout the application must be evaluated and tested for thread safety and side effects, including race conditions and deadlock scenarios. Additionally if the repository or business services even share any transient services, then thread safety can become an issue even between separate services.

Since singletons persist in memory for the lifetime of the application, this approach has also made the application susceptible to memory leaks, now it is every developers responsibility to make sure all objects are out of reference and cleaned up before exiting or they may persist in memory, growing with every request.

This approach has also decreased the applications testability and reliability, as testing multi-thread scenarios is more complex and challenging.

From James Michael Hickey's blog post discussing how singletons utilizing transitive services can allow completely different HTTP requests to add, remove and modify elements of each others collections:

https://www.blog.jamesmichaelhickey.com/NET-Core-Dependency-Injection/

Singletons + Transitive Services

A singleton, again, lives “forever”. It’s always the same instance.

Transitive services, on the other hand, are always a different instance when requested - or resolved.

So here’s an interesting question: When a singleton depends on a transitive dependency how long does the transitive dependency live?

The answer is forever. More specifically, as long as it’s parent lives.

Since the singleton lives forever so will all of it’s child objects that it references.

This isn’t necessarily bad. But it could introduce weird issues when you don’t understand what this setup implies.

Thread-Safety Perhaps you have a transitive service - let’s call it ListService that isn’t thread-safe.

ListService has a list of stuff and exposes methods to Add and Remove those items.

Now, you started using ListService inside of a singleton as a dependency.

That singleton will be re-used everywhere. That means, on every HTTP Request. Which implies on many many different threads.

Since the singleton accesses/uses ListService, and ListService isn’t thread-safe - big problems!

Be careful.

A strong case should be made for using a singleton, each case should be evaluated thoroughly before making the decision, there should be compelling reasons for its usage. A robust and well-supported argument must be presented in favor of utilizing a singleton in any given scenario.

If you want to constantly be putting out fires and want your application to be the talk of the organization, then by all means, use this approach.