Why is there a static/global lock on a lightinject Scope

[Kadajski]

I've been experiencing some issues recently related to Lightinject scope and the static readonly object disposableObjectsLock = new object() lock. I have a MVC application using the normal per-request scope in Lightinjec.Web but on my application startup I notice a large amount of requests waiting behind the lock for disposableObjectsLock.

Why is this lock static? Why don't we have a lock inside of each scope. Since the lock is only used to avoid concurrency issues with the HashSet<IDisposable> which is a local scoped object on the lightject Scope. We could change the lock to be a local variable on the Scope object too and this would avoid a lot of lock contention. Alternatively, if this was done to avoid allocations for additional disposableObjectsLock objects, we could just lock on the HashSet itself?

[seesharper]

Thanks for reporting this.
What tool did you use to monitor lock contention ?
I'm just looking for a way to reproduce the scenario you are describing.
On the top of my head I can't think of any reason the lock is static. Looking into it

[Kadajski]

Hey, thanks for the reply. The main tool I've used is to just take multiple memory dumps while this behavior is happening. Taken these dumps across different machines over many days now and in all of them every single thread is waiting behind this lock. The issue I'm having is only in production and all requests are waiting behind a lock so CPU goes up to 100% and all additional requests get queued by IIS. I haven't been able to replicate the same scenario on my local pc yet.

To be honest I am quite surprised as the operation in the lock should be really quick. Though when I looked at this code and saw that the lock was static it seemed like an easy win. My assumption is that due to the high number of requests + number of objects I generate each request it's causing a backup while everything waits behind this lock

[seesharper]

I'm trying to fire up a lot of threads and see how it looks in the Concurrency Visualizer in VS.
Would it be possible for you to try compiling without the static lock and see if that resolves the issue?

[Kadajski]

Yup I will do that and let you know. Thanks

[seesharper]

Thanks a lot. Appreciate it 👍

[seesharper]

Here are the results of trying to investigate this.

Created the following console app.

class Program
{
    static void Main(string[] args)
    {
        var container = new ServiceContainer();
        container.Register<Foo>(new PerScopeLifetime());
        using (var scope = container.BeginScope())
        {
            scope.GetInstance<Foo>();
        }

        List<Task> tasks = new List<Task>();

        for (int i = 0; i < Environment.ProcessorCount; i++)
        {

            tasks.Add(Task.Factory.StartNew(() =>
            {
                for (int j = 0; j < 1000000; j++)
                {
                    using (var scope = container.BeginScope())
                    {
                        scope.GetInstance<Foo>();
                    }
                }
            }));
        }

        Task.WaitAll(tasks.ToArray());
    }
}


public class Foo : IDisposable
{
    public void Dispose()
    {
        
    }
}

Then I ran the app using the Concurrency Vizualizer

https://marketplace.visualstudio.com/items?itemName=Diagnostics.ConcurrencyVisualizer2017

With static lock

With instance lock

I'm not an expert when it comes to interpreting the results of this tool, but the number of milliseconds spent on blocking has decreased dramatically in the second run so I guess that is a good thing 😄

[seesharper]

@Kadajski Any updates on this? Too soon to tell?

[wayneduke]

@seesharper unfortunately it just moved the contention to the lock in the CreateDelegate method, however from first impressions the problem we're experiencing is not as prevalent as it was before.

I'm not overly familiar with the LightInject codebase but I suspect our use case, which is a very large dependency graph (sometimes thousands of objects) per request and hundreds of concurrent requests, is going to be the root of our problems. We have some ideas on how we're going to fix (or minimize) the contention we're seeing so we will open another issue (or submit a PR) if there is something that can be improved in LightInject that would help our scenario.

[seesharper]

Yes, I am aware of the lock related to CreateDelegate. One way to overcome this is to do a GetService on all root services at startup warming up the container. Any suggestions as how to improve on this are appreciated 👍

Maybe implemented as an extension method

container.WarmUp();

[Kadajski]

Just to clarify my understanding here. From what I understand of the code the CreateDelegate only gets called once and its to create a delegate of how to construct the object when you resolve the type? This doesn't actually construct the object itself? The issue we have is that the application is really large and has a bunch of legacy code that has stuff like database calls in the constructor. So if we did a GetService for all registered types we may be doing a lot of additional work for types that we no longer actually use. Having a way to create all the delegates on startup without actually creating the objects though would be really useful

[seesharper]

You are right on the money with your understanding 😄
That is actually a very good idea. I'll look into it 👍

[dadhi]

Stealing the idea ;P

[seesharper]

We need to be a little careful here though. It should be narrowed down to the "root" services that is actually resolved through a delegate. If we just blindly create delegates for all services that is registered with the container, we will most probably create a LOT of delegates that are never executed. Bad for memory and possibly bad for service lookup later on.
You know what they say, "The road to hell is paved with good intentions" 😄
Identifying those root services might be easy in some scenarios and more complicated in others, but maybe start with identifying services that are most probably NOT root services.

For instance it should be fair to assume that a service injected into another service is not a root service.
Not saying that it could not be, but probably not.

On the other hand
A service never injected into other services is most likely a root service.

To illustrate this

public class Foo
{
   public Foo(Bar bar)
   {
   }
}

Register both services with the container

container.Register<Foo>();
container.Register<Bar>();

Resolve Foo

var foo = container.GetInstance<Foo>();

In this case we only create a delegate for Foo where the code for injecting Bar is inside the dynamic method pointed to by the Foo delegate. We never create a delegate for Bar unless it is resolved using GetInstance

So here, Foo is a root service where as Bar is not.

The first draft of something providing compilation of delegates.

void Compile(Func<ServiceRegistration, bool> predicate);

This allows the developer to filter which services should be "precompiled".

@Kadajski , @wayneduke Is this sufficient or would you expect LightInject to provide the list of root services or at least what it "thinks" are root services?

[Kadajski]

Hey @seesharper this seems amazing. I definitely think this is sufficient for our use-case. I actually think I prefer this to an approach of lightinject trying to figure out the list of roots. The application developer will know more than lightinject about the usages of the services and to try and cater for every scenario inside of the lightinject library is probably going to be really tricky to perfect. The predicate seems most flexible to all scenarios I can think of.

Surely you can only really know from lightinject the "roots" that exist from all the registered types at the time of "compilation", having an assumption that none of the child types will ever be created independently seems wrong to me. At least for most projects I have been involved on child types are often created independently of the root, so I'd still like to compile these independently

[dadhi]

The first draft of something providing compilation of delegates.

void Compile(Func<ServiceRegistration, bool> predicate);

What if the service is open-generic, and I want to precompile a specific closed variants (which is the only possibility)?

[seesharper]

container.Register(typeof(OpenGenericFoo<,>));
container.Compile<OpenGenericFoo<string, int>>();

[wayneduke]

@seesharper being able to compile these delegates is going to be a big win for us! I echo the sentiments of @Kadajski - leaving it up to the developer to provide a predicate gives the most flexibility.

[seesharper]

@wayneduke @Kadajski Wrapping up these changes now.

[seesharper]

Closed by #413 and #412

[seesharper]

@wayneduke @Kadajski LightInject 5.1.3 has just been published to the official NuGet feed introducing support for compiling delegates up front.
See here for more information http://www.lightinject.net/#compilation

This version also fixes the static lock in the Scope class

[wayneduke]

Thanks @seesharper! We were following and have already started preparing to release our app with the new version. We'll update and let you know if using the Compile method solves the contention issues we were seeing (which I'm sure it will).

[seesharper]

It should be quite interesting to see the effect of this change.Let me know if anything is unclear with regards to the API 👍

[seesharper]

@Kadajski @wayneduke Did you guys experience any improvement after upgrading to 5.1.3?

[Kadajski]

hey @seesharper we haven't been able to use the compile feature yet as we do actually need some changes in our application to get this working. Removing the static lock though does seem to remove that lock from showing up in any of our analysis though. The compile feature has helped a lot in exposing some of the issues in our application though.

To give some insight into our problems if you're interested or can give any advice:
When calling Compile on a type that is set to be PerContainerLifetime it seems like there's a switch in the lightinject code which actually causes this to create the object instead of just creating the delegate. Which leads back to the concerns I had earlier about a lot of objects getting created at startup which have a lot of work in the constructor and may not be used. I assume the reason this is done is because you don't need to create and store the delegate if the object will only ever need to be created once. We may copy PerContainerLifetime and create our own version of PerContainerLifetime in our app to avoid the object creation on startup as I see thats just an if(type is PerContainerLifetime) in code and instead only create it on the first call to GetInstance. Alternatively we may just fix our issue of having database work done in the constructor haha and try keep the built in library code

Another thing we found is that we're setting lightinject as the default dependency resolver in ASP.net mvc. This causes MVC to do a TryGetInstance for all the views(and other types) on startup. The views are never registered though so they always go through all the fallbacks in lightinject. One of them being AssemblyScanner.Scan which seemed to be a pain point from some snapshot data we had. May have just been coincidence though. Though since we know the views never will be registered we can just avoid calling into lightinject for this at all and try avoid all the fallbacks for this kind of stuff.

We also have a lot of our registrations chain factory resolutions. So what you mentioned before about only needing to compile the "root" types sadly isn't as simple in our case. For example we'd have something like:

container.Register<Foo>(f => new Foo(f.GetInstance<Bar>()))

So I'd assume we'll need to compile both Foo and Bar. Since we have this factory approach a lot though, a solution we're looking into is to make the root types be singleton/PerContainerLifetime and then just compile those. This way we can just reuse these objects instead of needing to build them up from the hundreds/thousands of dependencies they have down the chain each time