JasperFx / lamar

Fast Inversion of Control Tool and Successor to StructureMap
https://jasperfx.github.io/lamar
MIT License
568 stars 119 forks source link

Lamar 3.0.3 fails in multi threaded scenario #157

Closed danielpalme closed 5 years ago

danielpalme commented 5 years ago

If a class has a dependency on IEnumerable<IFoo> and you register several IFoo implementations, Lamar fails in a multi threaded scenario. You can get different problems:

The problem seems only to occur with version 3.0.3. In previous versions I did not experience the problem.

Here's a sample project to reproduce the issue: Sample.zip

jeremydmiller commented 5 years ago

@danielpalme Sorry this one slipped by me. I'll try to take a look at your sample project in the next couple days

jeremydmiller commented 5 years ago

Finally looked at this, can reproduce, but out of time. Might be after the 7/4 holiday now. Sorry for the long delay.

jeremydmiller commented 5 years ago

@danielpalme I have no earthly idea why this happens even after a couple hours of fighting with it. The super easy work around though is to just use arguments of type ISimpleAdapter[] or IReadOnlyList<ISimpleAdapter> and it works just fine.

I'm going to leave this open, but the real fix is probably just going to be some documentation.

CDargis commented 5 years ago

Bump. I believe we are seeing this issue with integration with MediatR: https://github.com/jbogard/MediatR/issues/438

I'm going to leave this open, but the real fix is probably just going to be some documentation.

Can we please try and dig into this one? I don't want to ask @jbogard to update a framework that is DI container agnostic.

jbogard commented 5 years ago

""""""Agnostic""""""

jeremydmiller commented 5 years ago

@CDargis I see absolutely nothing there about Lamar in your stacktraces. I'm gonna wait until it plays out on the MediatR issue.

CDargis commented 5 years ago

@jeremydmiller you aren't going to see any Lamar references in MediatR. Consumers can bring their own containers. This problem happens after Lamar has returned a list of instances. One item in this list is null hence the null reference exception seen in the linked issue. Sometimes there are duplicate items.

Does this help move the issue along? image image

public interface IClock
{
}

public class Clock : IClock
{
    public void Tick()
    {
    }
}

public class AnotherClock : IClock
{
    public void Tick()
    {
    }
}

class LamarSandbox
{
    public async Task StartAsync()
    {
        Lamar.Container container = new Lamar.Container(x =>
        {
            x.AddTransient<IClock, Clock>();
            x.AddTransient<IClock, AnotherClock>();
        });
        int times = 100000;
        List<Task> tasks = new List<Task>();
        for (int i = 0; i < times; i++)
        {
            Task t = new Task(() => Resolve(container));
            t.Start();
            tasks.Add(t);
        }
        await Task.WhenAll(tasks);
    }

    private void Resolve(Lamar.Container container)
    {
        IEnumerable<IClock> clocks = container.GetInstance<IEnumerable<IClock>>();
        if(clocks.Contains(null))
        {
            Console.Write("null!!");
        }
    }
}
CDargis commented 5 years ago

FWIW, I can reproduce in Lamar.Microsoft.DependencyInjection 3.0.0, 3.1.0 and 3.1.2. Can't seem to reproduce in 2.1.0.

jeremydmiller commented 5 years ago

Sigh, okay. MediatR has historically been a huge PITA for me supporting containers and I generally refuse to help support folks who get tangled up in it

Sorry, but I don't have enough bandwidth to fight with this one this week.

jeremydmiller commented 5 years ago

Do see the workarounds above though please

jbogard commented 5 years ago

Yeah don’t put this one on me lol I’m trying to keep things simpler by relying less on exotic features.

On Tue, Sep 10, 2019 at 5:50 PM Jeremy D. Miller notifications@github.com wrote:

Sigh, okay. MediatR has historically been a huge PITA for me supporting containers and I generally refuse to help support folks who get tangled up in it

Sorry, but I don't have enough bandwidth to fight with this one this week.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JasperFx/lamar/issues/157?email_source=notifications&email_token=AAAZQMTXL7F5QGZUKHIPHKDQJAQDBA5CNFSM4HLC6KJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6MXGTQ#issuecomment-530150222, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAZQMWJL7GRINQP3DHH76DQJAQDBANCNFSM4HLC6KJQ .

jeremydmiller commented 5 years ago

@jbogard It's not you man, it's the banana pants things that people do with generics trying to shoehorn crazy middleware strategies into MediatR that kills me

CDargis commented 5 years ago

Sorry, but I don't have enough bandwidth to fight with this one this week.

I can definitely empathize with lack of bandwidth.

Do see the workarounds above though please

I can't directly use those workarounds due to the way MediatR consumes containers.

@jbogard It's not you man, it's the banana pants things that people do with generics trying to shoehorn crazy middleware strategies into MediatR that kills me

You do realize this issue has nothing to do with generics or even MediatR? This is just container.GetInstance<IEnumerable<T>> failing in a multi-threaded scenario. I regret mentioning MediatR.

jeremydmiller commented 5 years ago

@CDargis MediatR in the past has been a huge PITA for me, as I've said a couple times. And it is related to what you're seeing here because of the way Lamar has to go discover the potential open generics/closed generics handlers inside of MediatR at runtime. The internal locking isn't quite right somehow, and that's the root cause of your issue here.

jorhar commented 5 years ago

For anyone who stumbles upon this thread experiencing a similar issue....we were unable to come to any type of resolution and unfortunately had to transition back to StructureMap.

As @jeremydmiller described above, there seems to be some issue with how Lamar resolves open generics or collections of open generics under heavy load...or something like that, and MediatR relies on open generic discovery.

At the time of this writing StructureMap has been sunsetted however. MediatR does include a very helpful Container Feature Support page.

jeremydmiller commented 5 years ago

I HAVE A FIX FOR THIS IN A LOCAL BRANCH!!!!!!!!!

This is finally getting taken care of in Lamar v3.1 out in the next 3-4 days.