JasperFx / lamar

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

Threading issues with DbContext #198

Closed jsheetzati closed 4 years ago

jsheetzati commented 5 years ago

I am having threading issues with DbContext + Lamar. Seeing a few errors, but this one is the most common A second operation started on this context before a previous operation completed. If I switch over to Microsoft DI, I do not have the same issue.

I created a reproducible sample here: https://github.com/jsheetzati/lamarExample. Currently, the project is setup to use Lamar DI, but I also have comments to switch the project over to Microsoft DI.

In order to run the project, please add a DB connection string to Context and SecondContext. I attempted to recreate the problem using the InMemoryProvider but was not having luck with that provider. If you use SQL Server, create a sample table in the database with the following definition:

create table Book
(
    Id int identity(1,1),
    Title nvarchar(30),
    Author nvarchar(30)
)

I used artillery to hit the API method with a bunch of requests simultaneously. npm install -g artillery artillery quick -n 5 --count 10 http://localhost:5000/Book/InsertAndReturn

Please let me know if you need any further information to reproduce the issue. Fairly confident at this point that Lamar is introducing a similar bug in one of our applications. There very well could be something wrong with our Lamar configuration as well, but I am not sure what that exactly is.

Thanks for all the hard work making this project. Looking forward to make the switch from StructureMap to Lamar :)

Edit: I am not sure if this is related to https://github.com/JasperFx/lamar/issues/173, but that is what initially prompted me to dig into our issue to see if it was something similar.

Package Information ASP.NET Core 3.0 Lamar 3.2.0 Lamar.Microsoft.DependencyInjection 4.0.0

saithis commented 5 years ago

We are in the process of upgrading to ASP.NET Core 3.0 and experience the same problem.

jensschmidbauer commented 5 years ago

Same error here. When doing like 10 parallel requests, it's quite likely that two of them get the same DbContext instance.

jeremydmiller commented 5 years ago

@jsheetzati If there's a next time, could you do a pull request for the repro steps in a failing xUnit test? Or could you do that instead, that's much easier than having to go clone, pull in other code, and recreate a failure case inside the Lamar codebase.

jeremydmiller commented 5 years ago

@jsheetzati Yeah, can you please try a pull request that reproduces this? The sample app isn't all that helpful. Sorry.

jsheetzati commented 5 years ago

Yea, I attempted to do that originally, but I wasn't having luck reproducing the issue with an InMemory DbContext. I cannot submit a PR with one of our Azure SQL connection strings. I'll try again reproducing with either InMemory or SQLite EF provider

jeremydmiller commented 5 years ago

I don't think you need anything with EF for that matter. We just need some way of proving that having any service marked as Scoped, but injected into a transient, is somehow not using unique instances per nested container.

jsheetzati commented 4 years ago

Sorry for the delay, I created a PR that I believe is demonstrating the issue. I was not able to duplicate the issue without also using ASP.NET Core 3. A simple unit test where I created containers + service scopes seems to work. Hope this helps.

https://github.com/JasperFx/lamar/pull/209

jasselin commented 4 years ago

I think we are experiencing the same behavior with NHibernate which is giving the following exception: "There is already an open DataReader associated with this Connection which must be closed first.".

Investigating a bit more, I find that the ISession's HashCode is used in two different threads when testing with concurrent requests.

rofr commented 4 years ago

@jasselin

"There is already an open DataReader associated with this Connection which must be closed first.".

This could be due to calling a method which starts a new query on the same connection, so not necessarily a concurrency issue. If your db is mssql then you would need to enable MARS (multiple active result sets) in the connection string.

And if the code is asyncronous, then it's perfectly normal to see different threads.

jasselin commented 4 years ago

@rofr I did try enabling MARS which fixed some problems with read operations. I was still having some odd behavior when doing writes.

I am pretty sure this is the same issue since the ISession hash code was the same between two threads.

Switching back to StructureMap made the problem disappear completely, even with MARS disabled.

rofr commented 4 years ago

I put a lock statement around the call to GetNestedContainer() and haven't seen any issues since. We made other changes as well so this might just have been a coincidence but sharing here anyway in case it might be helpful to someone.

jeremydmiller commented 4 years ago

I'm finally going to look at this again today.

jeremydmiller commented 4 years ago

FINALLY, there's some optimism on this one. Maybe, maybe the fix for #215 knocks this one out as well, but we'll see.