What's wrong with using ThreadStatic instead of a DI container

Question

I'm trying to make a very large, very legacy project testable.

We have a number of statically available services that most of our code uses. The problem is that these are hard to mock. They used to be singletons. Now they are pseudo-singletons -- same static interface but the functions delegate to an instance object that can be switched out. Like this:

class ServiceEveryoneNeeds
{
    public static IImplementation _implementation = new RealImplementation();

    public IEnumerable<FooBar> GetAllTheThings() { return _implementation.GetAllTheThings(); }
}

Now in my unit test:

void MyTest()
{
    ServiceEveryoneNeeds._implementation = new MockImplementation();
}

So far, so good. In prod, we only need the one implementation. But tests run in parallel and might need different mocks, so I did this:

class Dependencies
{
     //set this in prod to the real impl
     public static IImplementation _realImplementation;

     //unit tests set these
     [ThreadStatic]
     public static IImplementation _mock;

     public static IImplementation TheImplementation
     { get {return _realImplementation ?? _mock; } }

     public static void Cleanup() { _mock = null; }
}

And then:

class ServiceEveryoneNeeds
{
     static IImplementation GetImpl() { return Dependencies.TheImplementation; }

     public static IEnumerable<FooBar> GetAllTheThings() {return GetImpl().GetAllTheThings(); }

}

//and
void MyTest()
{
    Dependencies._mock = new BestMockEver();
    //test
    Dependencies.Cleanup();
}

We took this route because it's a massive project to constructor inject these services into every class that needs them. At the same time, these are universal services within our codebase that most functions depend on.

I understand that this pattern is bad in the sense that it hides dependencies, as opposed to constructor injection which makes dependencies explicit.

However the benefits are:
- we can start unit testing immediately, vs doing a 3 month refactor and then unit testing.
- we still have globals, but this appears to be strictly better than where we were.

While our dependencies are still implicit, I would argue that this approach is strictly better than what we had. Aside from the hidden dependencies, is this worse in some way than using a proper DI container? What problems will I run into?

Answer 1

Its a service locator which is bad. But you already know that. If your code base is that massive, why not start a partial migration? Register the singleton instances with the container and start constructor injecting them whenever you touch a class in your code. Then you can leave most parts in a (hopefully) working condition and get the benefits of DI everywhere else.

Ideally the parts without DI should shrink over time. And you can start testing right away.

Answer 2

This is called an ambient context. There is nothing wrong in using an ambient context if used and implemented correctly. There are some preconditions when an ambient context may be used:

1. It must be a cross cutting concern that returns some value
2. You need a local default
3. You have to make sure that null can not be assigned. (Use a Null implementation instead)

For cross cutting concerns that do not return values e.g. Logging you should prefer interception. For other dependencies that are not cross cutting concerns you should do constructor injection.

Your implementation has several problems though (does not prevent assigning null, naming, no default). Here is how you could implement it:

public class SomeCrossCuttingConcern
{
     private static ISomeCrossCuttingConcern default = new DefaultSomeCrossCuttingConcern();

     [ThreadStatic]
     private static ISomeCrossCuttingConcern current;

     public static ISomeCrossCuttingConcern Default
     { 
         get { return default; }
         set 
         { 
             if (value == null) 
                 throw new ArgumentNullException(); 
             default = value; 
         } 
     }

     public static ISomeCrossCuttingConcern Current
     { 
         get 
         { 
             if (current == null)
                 current = default; 
             return current; 
         }

         set 
         { 
             if (value == null) 
                 throw new ArgumentNullException(); 
             current = value; 
         } 
     }

     public static void ResetToDefault() { current = null; }
}

An ambient context has that advantage that you don't pollute your API for cross cutting concerns.

But on the other hand regarding testing you tests can become dependent. E.g. if you forget to setup your mock for one test it runs correctly if the mock was setup by another test before. But when it is run standalone or in a different order it will fail. It makes testing more difficult.