Possible race condition with default controller activator for Web API

[tonysneed]

First, congrats on the project - love it.

In exploring how scopes work and the Web API integration, I noticed what appears to be a possible race condition when using the default controller activator that comes with Web API. It seems like care should be taken that the controller for a request is paired with the dependency scope in an atomic fashion. Here is my fix:

public class ThreadSafeControllerActivator : IHttpControllerActivator
{
    private readonly object _syncRoot = new object();

    public IHttpController Create(HttpRequestMessage request, 
        HttpControllerDescriptor controllerDescriptor, Type controllerType)
    {
        // Beginning the scope and getting the instance need to be an atomic operation,
        // so that the controller created under this scope is disposed when 
        // the scope is disposed.
        lock (_syncRoot)
        {
            IDependencyScope dependencyScope = request.GetDependencyScope();
            var controller = (IHttpController)dependencyScope.GetService(controllerType);
            return controller;
        }
    }
}

What I would suggest is that container.EnableWebApi be modified to ThreadSafeControllerActivator, like so:

public static void EnableWebApi(this IServiceContainer serviceContainer, HttpConfiguration httpConfiguration)
{
    httpConfiguration.DependencyResolver = (IDependencyResolver)new LightInjectWebApiDependencyResolver(serviceContainer);
    IEnumerable<IFilterProvider> filterProviders = ServicesExtensions.GetFilterProviders(httpConfiguration.Services);
    httpConfiguration.Services.RemoveAll(typeof(IFilterProvider), (Predicate<object>)(o => true));
    httpConfiguration.Services.Add(typeof(IFilterProvider), (object)new LightInject.WebApi.LightInjectWebApiFilterProvider(serviceContainer, filterProviders));

    // Use thread safe controller activator to guard against race conditions
    httpConfiguration.Services.Replace(typeof(IHttpControllerActivator), new ThreadSafeControllerActivator());
}

Does this make sense? If so, I can create a pull request.

Cheers, Tony

[seesharper]

Hi Anthony and thanks for those kind words.

LightInject.WebApi has recently been updated to deal with a threading bug in the LightInjectWebApiDependencyResolver.

Maybe you could try out the latest bits from NuGet (LightInject.WebApi 1.0.0.3) and see if that solves your problem?

Best regards Bernhard Richter

On Sat, Sep 13, 2014 at 11:20 AM, Anthony Sneed notifications@github.com wrote:

First, congrats on the project - love it.

In exploring how scopes work and the Web API integration, I noticed what appears to be a possible race condition when using the default controller activator that comes with Web API. It seems like care should be taken that the controller for a request is paired with the dependency scope in an atomic fashion. Here is my fix:

public class ThreadSafeControllerActivator : IHttpControllerActivator{ private readonly object _syncRoot = new object();

public IHttpController Create(HttpRequestMessage request,
    HttpControllerDescriptor controllerDescriptor, Type controllerType)
{
    // Beginning the scope and getting the instance need to be an atomic operation,
    // so that the controller created under this scope is disposed when
    // the scope is disposed.
    lock (_syncRoot)
    {
        IDependencyScope dependencyScope = request.GetDependencyScope();
        var controller = (IHttpController)dependencyScope.GetService(controllerType);
        return controller;
    }
}}

What I would suggest is that container.EnableWebApi be modified to ThreadSafeControllerActivator, like so:

public static void EnableWebApi(this IServiceContainer serviceContainer, HttpConfiguration httpConfiguration){ httpConfiguration.DependencyResolver = (IDependencyResolver)new LightInjectWebApiDependencyResolver(serviceContainer); IEnumerable filterProviders = ServicesExtensions.GetFilterProviders(httpConfiguration.Services); httpConfiguration.Services.RemoveAll(typeof(IFilterProvider), (Predicate

[tonysneed]

Hi Bernhard,

I like the speed of LI, the simplicity of the design, and the documentation. Very nice. I am writing a course on Web API for Wintellect and have referenced it in the examples.

In terms of the threading issue, I'm looking at WebApi.cs, but I don't see anything there related to threading. Is there a GitHub issue that describes it?

What I'm talking about here has to do with how DependencyResolver is architected in Web API. You have to make sure that the controller for a request is the same controller that was created right after BeginScope was called.

Let me give you an example. Take a look at the Create method in my ThreadSafeControllerActivator shown above. Thread 1 enters the method and executes request.GetDependencyScope(). Thread 2 does the same. But instead of Thread 1 executing dependencyScope.GetService, Thread 2 gets there first. This is our race condition. In that case, when the scope created by Thread 1 is disposed, the controller created by Thread 2 will be disposed. That's not good.

This is not actually a bug in LightInject, but but it's a condition not anticipated by the default controller activator in Web API. Now, I could (and probably should) bring this up with the Web API team at Microsoft, because it's due to how IDependencyScope is designed. In other words, there's no way an implementation of IDependencyResolver can guard against it. This would not be the case if we placed the composition root in an IHttpControllerActivator (as suggested by Mark Seemann).

At any rate, while it's a bug in Web API, it can be defended against by LightInject in the manner shown above. Please let me know if this makes sense. :)

Cheers, Tony blog.tonysneed.com

[seesharper]

Hi Tony!

It was an issue with the LightInjectWebApiDependencyResolver that caused the wrong scope to be disposed under heavy load.

https://github.com/seesharper/LightInject/issues/97

I rather hastily concluded that it might be related to to this :)

I have always assumed that it was safe to follow the guidelines of how to create a custom dependency resolver for Web API ( http://www.asp.net/web-api/overview/advanced/dependency-injection) and never really considered the possibility that they have introduced the possibility of a race condition.

From what I can read in the DefaultHttpControllerActivator, the scope is tied to the current request and the logic for this is in the HttpRequestMessageExtensions class.

In your example we are most certainly talking about two different request and hence two scopes should be created.

But if we use your ThreadSafeControllerActivator as an example, it should be safe even here to remove the lock. There are no instance (or static) members here so we have no state and each thread has its own stack so that is safe too.

From what I can gather from this, I don't think Thread 2 can ever use a scope from Thread 1 since they represent two different requests.

I might be wrong about this, but it would be really interesting to hear what the Web API team has to say about this. I am not an expert on threading so again, I could be wrong about this.

Best regards Bernhard Richter

On Sun, Sep 14, 2014 at 4:53 PM, Anthony Sneed notifications@github.com wrote:

Hi Bernhard,

I like the speed of LI, the simplicity of the design, and the documentation. Very nice. I am writing a course on Web API for Wintellect and have referenced it in the examples.

In terms of the threading issue, I'm looking at WebApi.cs https://github.com/seesharper/LightInject/blob/master/LightInject.WebApi/LightInject.WebApi.cs, but I don't see anything there related to threading. Is there a GitHub issue that describes it?

What I'm talking about here has to do with how DependencyResolver is architected in Web API. You have to make sure that the controller for a request is the same controller that was created right after BeginScope was called.

Let me give you an example. Take a look at the Create method in my ThreadSafeControllerActivator shown above. Thread 1 enters the method and executes request.GetDependencyScope(). Thread 2 does the same. But instead of Thread 1 executing dependencyScope.GetService, Thread 2 gets there first. This is our race condition. In that case, when the scope created by Thread 1 is disposed, the controller created by Thread 2 will be disposed. That's not good.

This is not actually a bug in LightInject, but but it's a condition not anticipated by the default controller activator in Web API. Now, I could (and probably should) bring this up with the Web API team at Microsoft, because it's due to how IDependencyScope is designed. In other words, there's no way an implementation of IDependencyResolver can guard against it. This would not be the case if we placed the composition root in an IHttpControllerActivator (as suggested by Mark Seemann http://blog.ploeh.dk/2012/09/28/DependencyInjectionandLifetimeManagementwithASP.NETWebAPI ).

At any rate, while it's a bug in Web API, it can be defended against by LightInject in the manner shown above. Please let me know if this makes sense. :)

Cheers, Tony blog.tonysneed.com

— Reply to this email directly or view it on GitHub https://github.com/seesharper/LightInject/issues/106#issuecomment-55528013 .

[tonysneed]

I love these types of discussions - really gets you thinking. :)

LightInjectWebApiDependencyResolver correctly implements IDependencyResolver, although you really don't need LightInjectWebApiDependencyScope because IDependencyResolver inherits from IDependencyScope. This would be a more concise implementation:

public class LightInjectDependencyResolver : IDependencyResolver
{
    private readonly Scope _scope;
    private readonly IServiceContainer _container;

    public LightInjectDependencyResolver(IServiceContainer container)
    {
        _container = container;
    }

    private LightInjectDependencyResolver(IServiceContainer container, Scope scope)
    {
        _container = container;
        _scope = scope;
    }

    public object GetService(Type serviceType)
    {
        return _container.TryGetInstance(serviceType);
    }

    public IEnumerable<object> GetServices(Type serviceType)
    {
        return _container.GetAllInstances(serviceType);
    }

    public IDependencyScope BeginScope()
    {
        return new LightInjectDependencyResolver(_container, _container.BeginScope());
    }

    public void Dispose()
    {
        _scope.Dispose();
    }
}

Now, back to the race condition. In order to be scalable, Web API hosts need to be multi-threaded. In other words, if a host were to serve requests serially, it would be quite slow. ;) It's the same with WCF. So you could have two requests come into the controller activator, each on a different thread. If there is no lock in the Create method, then there is nothing to stop a context switch from taking place right after request.GetDependencyScope is called, in which case it is possible for Thread 2 to get to the next line of code, dependencyScope.GetService, before Thread 1 gets to it, for example, if Thread 1 spends more time in GetDependencyScope than Thread 2. Just imagine two threads racing each other.

So the lock is needed so that only one thread at a time can enter that block of code in the Create method. It has nothing to do with synchronizing access to shared state, as would be the case with either a static or instance member. And while each thread has its own stack, the scope and controller instances live on the heap because they are reference types. But again this doesn't have anything to do with the need for the lock, because we are using it to prevent the race condition, not synchronize access to a shared instance.

In terms of the default controller activator built by the Web API team, I'm not sure it should be responsible for guarding against the race condition, because the affinity between scopes and controllers may be something that is specific to each IoC container, which controls the behavior of disposing instances in the dependency tree when scopes are disposed. Either way, it is something that you can and should address. Other IoC container with libraries for Web API integration probably also suffer from this vulnerability, but I just so happen to be digging into LightInject and found it. Lucky you!!!

Cheers, Tony

[seesharper]

Hi Tony.

I'm going to investigate on this and try to force such a situation and see if I can make it fail somehow.

I'll even try to get more people to take a look at it :)

Best regards

Bernhard Richter

On Mon, Sep 15, 2014 at 10:47 AM, Anthony Sneed notifications@github.com wrote:

I love these types of discussions - really gets you thinking. :)

LightInjectWebApiDependencyResolver correctly implements IDependencyResolver, although you really don't need LightInjectWebApiDependencyScope because IDependencyResolver inherits from IDependencyScope. This would be a more concise implementation:

public class LightInjectDependencyResolver : IDependencyResolver{ private readonly Scope _scope; private readonly IServiceContainer _container;

public LightInjectDependencyResolver(IServiceContainer container)
{
    _container = container;
}

private LightInjectDependencyResolver(IServiceContainer container, Scope scope)
{
    _container = container;
    _scope = scope;
}

public object GetService(Type serviceType)
{
    return _container.TryGetInstance(serviceType);
}

public IEnumerable<object> GetServices(Type serviceType)
{
    return _container.GetAllInstances(serviceType);
}

public IDependencyScope BeginScope()
{
    return new LightInjectDependencyResolver(_container, _container.BeginScope());
}

public void Dispose()
{
    _scope.Dispose();
}}

Now, back to the race condition. In order to be scalable, Web API hosts need to be multi-threaded. In other words, if a host were to serve requests serially, it would be quite slow. ;) It's the same with WCF. So you could have two requests come into the controller activator, each on a different thread. If there is no lock in the Create method, then there is nothing to stop a context switch from taking place right after request.GetDependencyScope is called, in which case it is possible for Thread 2 to get to the next line of code, dependencyScope.GetService, before Thread 1 gets to it, for example, if Thread 1 spends more time in GetDependencyScope than Thread 2. Just imagine two threads racing each other.

So the lock is needed so that only one thread at a time can enter that block of code in the Create method. It has nothing to do with synchronizing access to shared state, as would be the case with either a static or instance member. And while each thread has its own stack, the scope and controller instances live on the heap because they are reference types. But again this doesn't have anything to do with the need for the lock, because we are using it to prevent the race condition, not synchronize access to a shared instance.

In terms of the default controller activator built by the Web API team, I'm not sure it should be responsible for guarding against the race condition, because the affinity between scopes and controllers may be something that is specific to each IoC container, which controls the behavior of disposing instances in the dependency tree when scopes are disposed. Either way, it is something that you can and should address. Other IoC container with libraries for Web API integration probably also suffer from this vulnerability, but I just so happen to be digging into LightInject and found it. Lucky you!!!

Cheers, Tony

— Reply to this email directly or view it on GitHub https://github.com/seesharper/LightInject/issues/106#issuecomment-55565266 .

[dotnetjunkie]

Hi Tony,

Did you actually run into this race condition in production, because I think you are actually wrong and there is no race condition. I've been looking at the Web API code using reflector and this looks fine to me.

The race condition you are talking about should not matter, since every request gets its own HttpRequestMessage and each message caches its own IDependencyScope. So each request gets its own dependency scope which means each request gets its own controller instance (resolved from its own IDependencyScope).

Since each request gets its own controller, each request will dispose its own controller. Obviously this goes wrong when two threads simultaneously call the Create method while passing in a reference to the same HttpRequestMessage instance, but that should be impossible, since each request gets its own HttpRequestMessage and requests are not handled in parallel; they are handled asynchronously.

[tonysneed]

Hi @dotnetjunkie !

So far this is just an interesting discussion about a possible race condition. :) Although, if the theory proves reasonable, the next step would be actually to produce the race condition in a unit test.

That said, my current thinking is that the race condition is not possible, but for different reasons than the ones you or Bernhard have given.

Let me first address your reasoning. It is not technically correct that "each message caches its own IDependencyScope." AFAIK, the IDependencyScope instance (in this case LightInjectWebApiDependencyScope) is a reference type that lives on the Heap and is therefore shared among multiple messages and multiple threads. The more important question is the storage location of LightInject.Scope, because controllers (and their dependencies) that are associated with this scope instance will be disposed when the scope is disposed. Therefore, your statement that "each request will dispose its own controller" is also technically incorrect, because the request disposes whatever was passed to RegisterForDispose, which in this case is LightInjectWebApiDependencyScope, which wraps a LightInject.Scope instance.

So, it turns out that what is most important here is that LightInject.Scope is not stored on the Heap. And I'm glad to report that it is not. It is in fact stored in Thread Local Storage, which means that whenever any thread calls GetInstance on a container that has PerScopeLifetime or PerRequestLifetime, then the instance will only be disposed when that thread's Scope is disposed. Problem solved!!!

I don't know why I didn't notice PerThreadScopeManagerProvider at first - I'm new to this IoC container, so I just needed to dig a bit deeper. Thank you, @seesharper and @dotnetjunkie, for your patience in satisfying my curiosity!

Cheers, Tony

[tonysneed]

Closing this issue, as it appears to be resolved.

[seesharper]

Thanks a lot both of you for valuable input. Best regards Bernhard

[dotnetjunkie]

Hi Tony,

There might be a little misunderstanding here. I have no knowledge about LightInject and in which way LightInject implements things and whether or not things might break by the way LightInject does things. You seem to indicate that there might be something wrong with the design of Web API that might cause a race condition (there's IMO a LOT wrong with Web API's design btw, but that's a different discussion). My feedback was purely focused on the Web API part of things, and after taking a close look at the Web API code base my conclusion is that there is no bug in Web API that could cause a race condition in the place you mentioned.

Perhaps the misunderstanding still continues here, because it might be that LightInject does things differently here, but it seem to me that you are mixing things up by saying: "the IDependencyScope instance ... is therefore shared among multiple messages and multiple threads". The 'thing' that is shared by all threads in Web API is the IDependencyResolver. When you call BeginScope, the IDependencyResolver class should create a new IDependencyScope instance that is SPECIFIC to that request. Controllers are resolved from that scope, not from the main resolver.

When you call the GetDependencyScope method on HttpRequestMessage (i.e. request.GetDependencyScope()), that extension method will check the message's Properties dictionary to see if that message already has an IDependencyScope instance. If it has, that instance will be returned from the extension method. If not, it will load the application's DependencyResolver and it will call BeginScope on that resolver. Begin scope will create a NEW IDependencyScope instance and this instance will be cached in the request's properties and returned from the GetDependencyScope method.

Whether or not instances live on the heap or not is as far as I see not relevant, since in .NET about everything lives on the heap, even structures often live on the heap. And the fact that things live on the heap, doesn't mean it is shared by multiple threads. As long as you don't publish a reference of such type to a different thread, those instances are thread-specific.

If the LightInject scope is stored in thread-local storage, as you seem to imply, there might in fact be big problems for LightInject users because Web API is truly asynchronous in nature. One single request will almost always flow from thread to thread and there will (almost) always be multiple threads involved when a single Web API request is processed. This means that one single request could access the scope that has been created for a different thread and that IS in fact really bad. So instead of using Thread-local storage, in case of Web API Execution Context should be used instead, since the Execution Context does flow with asynchronous methods and isn’t bound to a thread.

[seesharper]

I'll elaborate on this tomorrow, but lightinject has something called a PerCallContextScopeMangerProvider that enables scoping across async/await points.

On Monday, September 15, 2014, dotnetjunkie notifications@github.com wrote:

Hi Tony,

There might be a little misunderstanding here. I have no knowledge about LightInject and in which way LightInject implements things and whether or not things might break by the way LightInject does things. You seem to indicate that there might be something wrong with the design of Web API that might cause a race condition (there's IMO a LOT wrong with Web API's design btw, but that's a different discussion). My feedback was purely focused on the Web API part of things, and after taking a close look at the Web API code base my conclusion is that there is no bug in Web API that could cause a race condition in the place you mentioned.

Perhaps the misunderstanding still continues here, because it might be that LightInject does things differently here, but it seem to me that you are mixing things up by saying: "the IDependencyScope instance ... is therefore shared among multiple messages and multiple threads". The 'thing' that is shared by all threads in Web API is the IDependencyResolver. When you call BeginScope, the IDependencyResolver class should create a new IDependencyScope instance that is SPECIFIC to that request. Controllers are resolved from that scope, not from the main resolver.

When you call the GetDependencyScope method on HttpRequestMessage (i.e. request.GetDependencyScope()), that extension method will check the message's Properties dictionary to see if that message already has an IDependencyScope instance. If it has, that instance will be returned from the extension method. If not, it will load the application's DependencyResolver and it will call BeginScope on that resolver. Begin scope will create a NEW IDependencyScope instance and this instance will be cached in the request's properties and returned from the GetDependencyScope method.

Whether or not instances live on the heap or not is as far as I see not relevant, since in .NET about everything lives on the heap, even structures often live on the heap. And the fact that things live on the heap, doesn't mean it is shared by multiple threads. As long as you don't publish a reference of such type to a different thread, those instances are thread-specific.

If the LightInject scope is stored in thread-local storage, as you seem to imply, there might in fact be big problems for LightInject users because Web API is truly asynchronous in nature. One single request will almost always flow from thread to thread and there will (almost) always be multiple threads involved when a single Web API request is processed. This means that one single request could access the scope that has been created for a different thread and that IS in fact really bad. So instead of using Thread-local storage, in case of Web API Execution Context should be used instead, since the Execution Context does flow with asynchronous methods and isn’t bound to a thread.

— Reply to this email directly or view it on GitHub https://github.com/seesharper/LightInject/issues/106#issuecomment-55659260 .

[tonysneed]

Hi @dotnetjunkie ,

Whoa, you are a .NET junkie. :) I have to say that I am one too, having worked as an instructor for DevelopMentor for several years now, and I'm also on staff at Wintellect. That doesn't mean I know everything, or that I can't be wrong from time to time. ;-) But it explains why I really like discussions about stacks, heaps, GC, and threading, etc. And if needed, I can also ask some other guys for their opinion. I'm into Web API at the moment because I'm authoring a course on it and have been digging into the internals. And you're right about shortcomings in Web API, but I'm wondering if some of those might be rectified in vNext? I'd love to hear your thoughts on that.

At the beginning of this discussion, I was concerned that a thread could ask a container for a controller instance within a scope that belonged to another request. But I can see now that, at least with LightInject, that's just not possible. Because LightInject stores its Scope object in thread local storage, it can only be owned by a single thread. When that thread asks the container for a controller instance (or one of its dependencies), there is a relationship created between the instance and its owning Scope, such that disposing the Scope will dispose the instance. This behavior is specific to the PerThreadScopeManagerProvider, which is the default. As @seesharper mentions, there is also a PerLogicalCallContextScopeManagerProvider which uses Execution Context for scoping across async continuations.

I appreciate your description of how IDependencyResolver.BeginScope returns a new IDependencyScope that is cached with the request, but the thing to watch out for is that this scope object is usually passed a reference to the container, which lives on the Heap and may or may not be thread safe. But as I mentioned, my concerns about the race condition were unfounded.

Now, I would like to comment on just a couple things you said, mainly just for academic sake. :) This statement seems off to me: "And the fact that things live on the heap, doesn't mean it is shared by multiple threads. As long as you don't publish a reference of such type to a different thread, those instances are thread-specific." According to my knowledge of the Heap, objects living on the heap can pretty much always be accessed by multiple threads (unless they are making use of TLS). This is why reference types are considered not thread-safe by default and will need some synchronization device to enforce thread safety - Monitor, Interlocked, Mutex, Semaphore, etc. So I'm not sure what you mean by "publish a reference of such type to a different thread"?

The other statement I'd take issue with is: "one single request could access the scope that has been created for a different thread." Because a Scope is placed in TLS, each thread gets its own instance of a Scope, and it is not shared with other threads at all.

That's it for tonight. Time for bed!

Cheers, Tony

PS. I apologize if I'm coming across as cheeky. You guys obviously know what you're talking about ... It's just the trainer in me that enjoys the give and take of a lively discussion and what I hope to learn from it. :)

[seesharper]

No problem, Tony. I consider this a very valuable discussion. :)

There are basically three class types involved in the scoping mechanism in LightInject.

• Scope (The actual scope)
• ScopeManager (The class that creates and disposes the Scope)
• IScopeManagerProvider (Represents a class that "provides" a ScopeManager)

The default implementation of the IScopeManagerProvider is the PerThreadScopeManagerProvider that returns a IScopeManagerProvider per thread. This implementation works great for most cases, although there are situations where it does not which I talk about later.

The Scope itself holds a reference to the ScopeManager that created it which effectively means that the Scope could start on one thread and finish on another.

Anyway, when we are talking about Web API it sort of depends on the hosting environment. IF we are hosting on IIS (System.Web), LightInject has a dependency on LightInject.Web that uses the PerWebRequestScopeManagerProvider. This provider stores the ScopeManager in the current HttpContext and basically gives you scoping per web request. The PerThreadScopeManagerProvider cannot be used here because of something that is commonly referred to as ASP.Net thread agility. This means that for instance in MVC, the request might start on one thread and switch to another thread by the time the IDependencyResolver (MVC) gets invoked.

Given that our hosting environment is IIS we then need to ensure that HttpContext.Current is transferred across await points. This can be done by setting the aspnet:UseTaskFriendlySynchronizationContext value to true.

HttpContext.Current.Items after an Async operation

When we are self hosting Web API, there is no HttpContext and we need another mechanism for providing the ScopeManager. As mentioned before, LightInject defaults to the PerThreadScopeManagerProvider which is fine for the majority of cases given that fact that LightInject allows a Scope to be ended on a different thread.

But there are cases where even this breaks down and that is when we are requesting something from the container inside an asynchronous method running on another thread.

Let's illustrate this by creating a simple console application.

using System;
using System.Threading.Tasks;
using LightInject;

class Program
{                
    static void Main(string[] args)
    {
        var container = new ServiceContainer();
        container.Register<IBar, Bar>(new PerScopeLifetime());
        container.Register<IFoo, Foo>();

        using (container.BeginScope())
        {
            var foo = container.GetInstance<IFoo>();
            ExecuteAsync(foo).Wait();
        }
    }

    private async static Task  ExecuteAsync(IFoo foo)
    {
        await Task.Delay(200);
        foo.Execute();
    }
}

public interface IBar {}

public class Bar : IBar { }

public interface IFoo
{
    void Execute();
}

public class Foo : IFoo
{
    private readonly Lazy<IBar> lazyBar;

    public Foo(Lazy<IBar> lazyBar)
    {
        this.lazyBar = lazyBar;
    }

    public void Execute()
    {                        
        IBar bar = lazyBar.Value;
    }
}

The IBar service is registered with the PerScopeLifetime and hence it need an active scope to be created. By the time we enter the Foo.Execute method we are on another thread and the container looks for a ScopeManager on that thread. But the scope is created on the main thread so there is no active scope here. Because of this, LightInject throws the following exception if we try to run this application.

Attempt to create a scoped instance without a current scope.

To deal with these situations, LightInject offers the PerLogicalCallContextScopeManagerProvider that uses the CallContext class to store/provide the ScopeManager.

container.ScopeManagerProvider = new PerLogicalCallContextScopeManagerProvider(); 

Once this is enabled, the ScopeManager instance is now transferred across await points and our application runs just fine.

LightInject uses the LogicalThreadStorage<T> class for this that also could be used for other values that needs to be stored per call context.

So why is this not the default?

According to Stephen Cleary and this article, using the CallContext can only be used in a safe way when running on .Net 4.5 or later. Using the CallContext also brings in some additional overhead and for these reasons I have decided this to be opt-in rather that opt-out.

[dotnetjunkie]

Hi Tony,

Whether or not objects on the heap are shared by multiple threads depends on the code written. Replace the word 'publish' with 'shared' and replace the words 'type' with 'instance'. So read my comments as "As long as you don't share a reference of such instance to a different thread, those instances are thread-specific". Take a look at the following example:

class MutableMe { public DataTime Value; }

public void DoSomething()
{
    var x = new MutableMe();
    x.Value = DateTime.Now;
    x.Value = DateTime.Now;
}

Here MutableMe is a class with some state. I choice this class explicitly since: 1. it is a class and its data will therefore be stored on the heap. 2. It has a struct value (of DateTime) and since the class is stored on the heap the value of this struct will be as well. 3. The class is mutable and its value can be changed. 4. Changing a DateTime value is not thread-safe, since there can be a thorn write, even on 64 machines (according to CLR and C# specifications).

However, although the MutableMe instance is stored on the heap, and MutableMe is changed multiple times, this example is thread-safe, since the reference to the MutableMe instance is not shared with other threads and no concurrent read/writes are done. If another thread calls DoSomthing, it will get its own value, which isn't shared, and the complete thing is thread-safe.

Now take the following example:

public async Task DoSomething()
{
    var x = new MutableMe();
    await Task.Delay(1);
    x.Value = DateTime.Now;
    await Task.Delay(1);
    x.Value = DateTime.Now;
}

Now DoSomething is an asynchronous method and now the MutableMe value is accessed from multiple threads. Still there is no problem though, since all data just moves from one thread to the other and values are not accessed concurrently.

But obviously, this will start breaking down when we do something like this:

private static MutableMe x = new MutableMe();

public void DoSomething()
{
    x.Value = DateTime.Now;
    x.Value = DateTime.Now;
}

Because now the same instance is shared over threads that can access it concurrently. And in this case we need to start synchronizing access using monitors, interlocked, etc.

So I hope that the text above proved that the mere fact of objects being on the heap doesn't mean things are unsafe. As a matter of fact, we can exaggerate things by making the class immutable:

class ImmutableMe { 
    public readonly DataTime Value; 
    public ImmutableMe(DateTime value) {
        this.Value = value;
    }
}

private static ImmutableMe x = new ImmutableMe(DateTime.Now);

public void DoSomething()
{
    x = new ImmutableMe(DateTime.Now);
    x = new ImmutableMe(DateTime.Now);
}

By doing this we ensure that the DateTime value will never be thorn, because although it can be read concurrently, it is only read after it is 'published'. This means, only after ImmutableMe is created on the heap and the Value field is written by that single thread, the reference to ImmutableMe is becoming available (publish) for other threads to read. This operates on the assumption that reads and writes will not be reordered across the constructor call, but the memory model of the CLR does ensure this. Otherwise we would be in a complete hell and even all immutable data structures that Microsoft creates for .NET, and all immutable types of F#, run on that assumption as well.

Do note though that most immutable data structures that Microsoft created with both C# and for instance the immutable collections are reference types. Immutable types don't need synchronization, since they can't be changed after creation and they can be safely shared across threads. So saying the reference types are not thread-safe by default is not correct. I would say that any mutable types (no matter whether it has by value or by reference semantics) are not thread-safe by default.

But when you start using thread-local storage across async/await, the trouble can start. Take a look at the following example:

[ThreadStatic]
private static MutableMe x;

private MutableMe GetValue() {
    if (x == null) x = new MutableMe();
    return x;
}

public async Task DoSomething()
{
    MutableMe x = GetValue();
    await Task.Delay(1);
    x.Value = DateTime.Now;
    await Task.Delay(1);
    x.Value = DateTime.Now;
}

Here we defined the MutableMe variable as ThreadStatic (thus using TLS) and we have a GetValue() method that does a lazy initialization of that thread-specific field. But we use this value in an asynchronous method and this can lead to concurrency bugs in this case. Read along to see what happens here.

Let's say there are two requests coming into the system that uses an asynchronous pipeline (just as Web API does). Let's call them request A and B. Let's say that request A is started one millisecond before B and both call the DoSomething() method. When request A is started, it starts to execute on thread X. During the execution on thread X, DoSomething() is called and a MutableMe instance is created and stored for thread X. After that request A gets delayed for one millisecond (using Task.Delay to simulate some I/O). When this happens thread X becomes free to handle any other requests. And that's the time that request B comes in and request B also starts executing on thread X. Note that since request B starts executing on thread X, it will get a hold on that same MutableMe instance as request A did, simply because it runs on that same thread X. Now the reference of the MutableMe instance, which is returned by GetValue(), is stored by the DoSomething method in a closure for later use when it should 'continue with' the rest of the code after the one millisecond delay. The same happened for request A of course.

Now hopefully you'll see the problem emerging. Both requests are referencing the same mutable value instance. Up until now everything still runs smoothly, but after this, things will start breaking down, because now request A continues and the first free thread is picked up, again thread X. But at almost the same time request B starts running again, but since thread X is taken, it now continues on thread Y. And now we have two different threads that are writing to the same instance concurrently and we have a race condition.

But even if they don't write concurrently, we'll probably have a problem because two requests use the same instance. This could happen if you store the scope in TLS, since it is possible that both requests use the same scope, which could result in them using the same controller or some other type that is cached per scope, or causing one request to dispose that scope, while the other is still running on it. Just like in Web API, a scope in LightInject is probably specific to one single request, but if you store that in TLS, it might be possible that two requests get the same instance. Whether or not this is actually possible depends on the implementation, but I hope I described that this could be a real problem.

So moral of the story is: don't use TLS when asynchronous method calls are involved.

You can still use this in case there's no async/await involved, but that is a scenario that you won't see that often. Async/await cuts through your whole application and almost every method must return Task.

I hope this helps.

[seesharper]

Great explanation, Steven. You obviously know the field of threading by heart :)

What do you think of using the CallContext class on .Net 4.0 It seems like Stephen Cleary suggests that this can only be "trusted" on .Net 4.5 or higher.

Another interesting challenge is what one should do on WinRT where there is no CallContext and where async/await is very relevant.

PS:Really appreciate you taking the time to fill in the blanks :)

[dotnetjunkie]

Hi Bernhard,

No problem. As Tony said, I'm a .NET Junkie :-) I love discussing, talking about and tinkering with this stuff. There's so much still to learn and these conversations really help.

I'm by no means an expert in the field of multi-threading. I like to say that I know just enough to keep myself out of trouble. For Simple Injector the whole async/await support was made in association with Blueling, who had much more experience with that whole model, but still we read Stephan Cleary's articles over and over again to make sure that our assumptions were right.

For Simple Injector we explicitly decided to only support async/await on .NET 4.5 because of the differences between 4.0 and 4.5. The reasoning behind this can be found in this SO answer.

The lack of having a CallContext in WinRT is very annoying, because this practically means you have to be very careful when doing async/await with your DI container. This basically means that you are forced to resolve the whole object graph at once (so no dependency delaying stuff like factories and Lazy in the graph) or you will have to make sure you pass on the Scope object throughout the graph (which pretty sucks, because this leaks the container implementation through your code and leads to very ugly code).

So as you might imagine, Simple Injector's ExecutionContextScopeLifestyle does not support WinRT, simply because there is no CallContext. This is really really sad.

[tonysneed]

Hello!

I had hoped to respond yesterday, but thanks for your patience. I appreciate the lucid explanation from @dotnetjunkie - I can now understand much more clearly what you are saying. :) Each thread calling MutableMe.DoSomething gets a new instance of MutableMe which is not accessible because it only has a local reference in the method frame (and not a member reference). It's obvious that an object on the heap can only be accessed my multiple threads concurrently if there is a reference accessible to those threads, but I very much like your explanation and the discussion of how to achieve this with immutable types.

Your discussion of reentrancy with TLS is quite relevant in this context, because the default PerThreadScopeManagerProvider seems to store Scope in TLS, and multiple requests (running on the same thread) could reference the same Scope! If they do, then calling Dispose on that Scope will dispose controllers belonging to different requests, which would not be good.

So it seems like there may be a design issue with regard to how the LightInject container associates instances with scopes. The default behavior is to associate a controller and its dependencies with the Scope that is in TLS at the time the request is made for a controller from the container. It seems like what is needed is an overload of GetInstance that accepts a specific Scope. If that is the Scope that belongs to a specific HttpRequestMessage, then we can always be assured that disposing that Scope will only dispose the controller associated it. And there is no need for an IHttpControllerActivator which blocks the thread while Scope and controller are being married, provided that we get a new Scope every time that GetDependencyScope is called and that this Scope is stored in a location where only the owning HttpRequestMessage can get it.

Furthermore (please correct me if I am wrong about this), I don't think PerLogicalCallContextScopeManagerProvider is going to help us here. I briefly took at look at how CallContext works, and it seems like storage is within the context of a particular method call, so that state persists across continuations (async / await). That pertains to calling GetInstance to associate an instance with a Scope in the CallContext, whereas what needs to be achieved here is associating an instance with a Scope that is specific to an HttpRequestMessage.

UPDATE: It looks like CallContext does seem to flow throughout a request in Web API, as described by @dotnetjunkie here.

See my comment below for further thoughts on this ...

Cheers! Tony

[seesharper]

Hi Tony.

I don't have a lot of time today to go deep into this, but this question on SO might be worth a look.

http://stackoverflow.com/questions/22931946/can-data-stored-in-callcontext-leak-between-requests

Bernhard

[tonysneed]

Thanks Bernhard for your reply. As you can see from my edited comment, it looks like a CallContext is used throughout the lifetime of a Web API request. My concern, however, is that the default behavior of LightInject is to use per thread scoping. I'm wondering if you could configure the ServiceContainer in the UseWebApi method to use PerLogicalCallContextScopeManagerProvider?

[seesharper]

In light of the latest information provided here, that seems to be a good default. At least way better than TLS. Question is whether this should be the default regardless of Web API. Async/await on a client desktop application for instance.

On Wednesday, September 17, 2014, Anthony Sneed notifications@github.com wrote:

Thanks Bernhard for your reply. As you can see from my edited comment, it looks like a CallContext is used throughout the lifetime of a Web API request. My concern, however, is that the default behavior of LightInject is to use per thread scoping. I'm wondering if you could configure the ServiceContainer in the UseWebApi method to use PerLogicalCallContextScopeManagerProvider?

— Reply to this email directly or view it on GitHub https://github.com/seesharper/LightInject/issues/106#issuecomment-55915094 .

[tonysneed]

Would changing the default possibly break existing code? I'd be interested in what @dotnetjunkie thinks about it.

[seesharper]

There is always a chance for breaking something when changing defaults:) I'll do it for Web API first.

On Wednesday, September 17, 2014, Anthony Sneed notifications@github.com wrote:

Would changing the default possibly break existing code?

— Reply to this email directly or view it on GitHub https://github.com/seesharper/LightInject/issues/106#issuecomment-55931005 .

[dotnetjunkie]

I know too little of the LightInject code base to really comment on that. I can explain however the approach that Simple Injector takes. As I understand correctly from the discussion above, the LightInject container is configured with a default scoping mechanism. In Simple Injector the scoped lifestyles live in separate assemblies and the core library knows nothing about them (accept that the core library defines the ScopedLifestyle base class). You'll have to use one of the ScopedLifestyle derivatives (from the integration packages) to do either TLS scoping or Call Context scoping, so introducing the ExecutionContextScopeLifestyle was never a breaking change in Simple Injector. But more importantly, making the Call Context the default scoping mechanism, might disallow using LightInject in .NET 4.0, WinRT or other platforms (that painfully lack the Call Context API). That would probably not be a good idea. So a better solution might be to have no default scoping at all and force users to configure it using one of the LightInject integration packages. You can throw an exception if someone makes a scoped registration when no scopingprovider is configured.

The fact that Simple Injector not integrates scoping into the core library of course has its downsides, because it becomes less intuitive for users to use scoping, since you always need an extra package for that. However, I'm very glad that I never integrated the TLS lifestyle into the core library, since it's becoming more obsolete by the day including it in the core library might push developers into the wrong direction.

In Simple Injector, getting the correct scope for the current context is always something that is done transparently on the background for the user. So you will never have to pass a scope on. If I understand correctly, LightInject takes the same approach as Simple Injector. In my vision this is the best approach, because this makes most scenarios much easier compared to having to resolve from the scope (as Autofac does). In this respect, it seems unpleasant to have an GetInstance overload that accepts a scope; the GetInstance method should be able to find the scope for the user and this is no problem when using Call Context.

[seesharper]

When I said that code might break, I meant that there might be code out there that depends on the default behavior with regards to scoping. Changing the default for LightInject itself is not going to break anything with respect to the internals of LightInject. But as Steven points out, CallContext is not available on WinRT. The LightInject build system spits out platform specific binaries so that it would be possible to provide different defaults for each platform. In practice this is not big issue yet as we all know that "metro style" apps is not exactly taking the world by storm. Neither does Windows Phone, at least not yet. The async/await pattern renders TLS pretty much useless for a majority of cases and based on the fact the async/await gets adopted into more applications, it might be a good argument for changing the default so that CallContext is used for scoping.

And as for the suggestion for passing a scope with the GetInstance method, I would have to agree with Steven that this should be transparent to this method. Unless we are doing the service locator (anti)pattern, we actually have very few places in our application where we manually invoke the GetInstance method. That is typically a top-level call and the container takes it from there. Producing instances further down in the object graph requires access to the scope and based on the current state of things in .Net, CallContext is out best bet. What I would like to see though is something a little more lightweight that for instance does not live in the Remoting namespace and something works equally well on all platforms. I'm doing iOS and Android support right now and only God knows what the story is with CallContext looks like on these platforms :)

[tonysneed]

I agree with both of you that passing Scope to GetInstance is not really a good idea - that was just conjecture on my part. More generally, the question is about whether to pick up an ambient scope, versus specifying the scope more explicitly. The style of TransactionScope is the ambient strategy, hence its use of TLS, but it harks from the days of .NET 2.0, and async / await constructs have thrown a monkey wrench into that pattern (although they just take something that was a pain to do and make it a natural part of the language, so threading was an issue with TransactionScope even back in 2005).

The advantage of an ambient scope is not really having to think about it too much. Just stick it in a using block and you're done. But, as Steven points out, the issue with a default scope using CallContext is that it's not supported on all platforms, notably .NET 4 and Win RT, and probably not supported by Xamarin either. (A buddy of mine heads up Xamarin U, so I can ask him about it.) Personally, I would not be a fan of different defaults depending on platform, so I would advise against that approach. Seems somewhat shaky to have to know what platform you're on to figure out what scope storage mechanism is used.

So I would recommend moving from ambient scope to a model where it is specified explicitly. That would be a breaking change, but a good one I think. For SimpleInjector it's specified when you register services. Another approach might be to add a parameter to BeginScope, specifying the lifetime. You'd want to do it in an extensible manner, so that it would be easier to add other scope lifestyles later. That way, if someone wants to use a Per Thread lifestyle, no problemo. If they want to use Per Call Context, easy peasy. To avoid confusion, I would add a specification for WebApi, which under the covers is use Per Call Context (I think SimpleInjector takes this approach).

PS. I'm also a fan of SimpleInjector, having used it with a very large WCF service bus implementation, where performance was critical. And I created a thing called Common Instance Factory, which was mainly an IoC abstraction for WCF, although I'm now persuaded that such adapters are not really a great idea in general, as they tend to fall into either service location or big container anti-patterns.

[seesharper]

There is a reason for not specifying exactly what type of "scope provider" to be used when registering services. The container does not care or know for that matter where the scope comes from. The PerWebRequestScopeManager for instance, that is used for providing scopes per web request, stores the Scopemanager in HttpContext.Current.Items and works great for web applications running on IIS.

Say that we have a simple Composition root

public class CompositionRoot : ICompositionRoot
{
    public void Compose(IServiceRegistry registry)
    {
        registry.Register<IFoo, Foo>(new PerScopeLifetime());
    }
}  

All we are saying here is that the services should be scoped, not how, where or when. This in turn means that we can easily reuse this composition root say in a unit test scenario without being bothered with mocking HttpContext which is a real pain. We can just have the test configure the container from the same composition root that the production code uses and handle the scoping in the test using another ScopeManagerProvider.

It should be noted though that there is nothing that prevents users from implementing their own ILifetime and pass that along to the register method.

But as mentioned before, the current state of .Net is most certainly moving into "async" mode and that means that CallContext (or anything similar to that) is probably the only "scoping manager" we need.

That being said, the dependency injection story on ASP.NET and related technologies (MVC, Web API, SignalR, ..) are somewhat fragmented and they each have their own abstraction with regards to DI. This will all change with ASP.vNext where all technologies share the same DI abstraction. That in combination with the OWIN that seems to be adopted everywhere we might end up with a web stack that has an uniform DI story and has no bindings to System.Web.

As for LightInject, I do not want to introduce any breaking changes at this point in time, but I will document the implications of using each ScopeManagerProvider. LightInject.WebApi will be modified to use the PerCallContextScopeManagerProvider as the default because of the implicit async nature of Web API.

Right now, the focus is to offer support for iOS and Android (Xamarin) so that users can enjoy the same API across all platforms.

[tonysneed]

Sounds like a good plan. Appreciate keeping things light. :) this discussion has been quite enlightening. Much thanks!=

[dotnetjunkie]

The path Simple Injector takes in making the Composition Root reusable over application types is by using the ScopedLifestyle base class. Bernhard's example would look like this in Simple Injector:

public static class CompositionRoot
{
    public static void Compose(Container container, ScopedLifestyle scopedLifestyle)
    {
        container.Register<IFoo, Foo>(scopedLifestyle);
    }
}

Just as with LightInject we, "all we are saying here is that the services should be scoped, not how, where or when". This way the running application (or unit test) can determine what type of lifestyle should be used:

var container = new Container();

CompositionRoot.Compose(container, new WebApiRequestLifestyle());

Or use some hybrid lifestyle:

var container = new Container();

ScopedLifestyle hybridLifestyle = Lifestyle.CreateHybrid(
    lifestyleSelector: () => HttpContext.Current != null,
    trueLifestyle: new WebRequestLifestyle(),
    falseLifestyle: new LifetimeScopeLifestyle());

CompositionRoot.Compose(container, hybridLifestyle);        

Or inside a unit test:

var container = new Container();

CompositionRoot.Compose(container, new FakeScopeLifestyle(new Scope()));

So it is possible to have the same reusability of your composition root, even without typing the scope onto the container instance, although you might be forced to pass some object on to the reused part of the composition root.

Of course Bernhard, since I'm giving Simple Injector's intimate details away, I hope you understand I have to kill you :-D

About the uniform DI strategy for ASP.NET, Mark Seemann wrote an interesting peace (http://blog.ploeh.dk/2014/05/19/conforming-container/) where he warns about what Microsoft is doing in this direction. I agree with him on that.

[tonysneed]

Things are becoming clearer now, and I think these explanations will benefit others as well. :) The best approach is, as both of you described, to abstract the details of scope storage away from the composition root, which just cares about registering types with some sort of lifestyle. In addition, the gory details of scope storage should also be abstracted from the application, which selects a lifestyle which is appropriate to the environment (for ex, System.Web, WebAPI, OWIN, WinRT, Xamarin, etc). This is why I like Bernhard's proposal to move the selection of storage manager provider into the integration library (I presume by setting the ScopeManagerProvider on ServiceContainer to PerLogicalCallContextScopeManagerProvider in the EnableWebApi extension method), and I would encourage this approach with the other integration libs for LightInject.

One take away from this discussion might be that per thread scope is probably not such a great idea. There has got to be another way to deal with scope storage. I like what Steven has done with "scope lifestyle" and how the concrete lifestyle is specified by the application (perhaps via integration package) when passed to the composition root (which just deals with the base class).

I haven't had a chance to look at ASP.NET vNext too closely - other than Scott Hanselman's Tech Ed talks. It will be interesting to see what approach it takes with the shared DI abstraction, but we may end up with a service locator anyways.

Cheers, Tony