zcz527 / autofac

Automatically exported from code.google.com/p/autofac
Other
0 stars 0 forks source link

Multithreaded resolution throws ArgumentException or NullReferenceException #373

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. Create two containers in the same process with dependencies that use 
constructor injection.
2. Resolve a dependency with constructor injection concurrently (multiple 
threads).
3. Boom! Either NullReferenceException or ArgumentException. If it doesn't fail 
at first, keep trying (a bigger loop ;-))

What is the expected output? What do you see instead?
Duh. No exception, of course.

What version of Autofac are you using? On what version of .NET/Silverlight?
Autofac 2.6.2.859, .NET 4.0

Please provide any additional information below.
Granted, multiple container instances inside the same process space might be 
somewhat of an anti-pattern. I just happen to be co-hosting two otherwise 
isolated services inside the same process space. I would expect two containers 
inside the same process space (appdomain to be more correct) to be isolated. 
Statics are evil. The exception callstack origin is observed in 
Autofac.Core.Activators.Reflection.ConstructorParameterBinding.Instantiate. The 
culprit is the static _contructorInvokers dictionary that walks into the 
multi-threaded minefield. I'd suspect concurrent resolutions on a single 
container to produce the same behavior.

I'd provide you with a patch, but it's unclear to me if the source up here is 
the latest nor what branch/version I should be working on. The quick'n'dirty 
solution is turning it into a ConcurrentDictionary for .NET 4.0. The long term 
solution seems a more explicit form of container local caching. Also, could the 
caching not be done any earlier (with associated trade-offs)?

Original issue reported on code.google.com by yves.rey...@gmail.com on 8 Jun 2012 at 9:06

GoogleCodeExporter commented 8 years ago
I'd like to contribute towards fixing this, but then I'd need the code that 
corresponds to 2.6.2.859. Can someone point me in the right direction?

Original comment by yves.rey...@gmail.com on 9 Jun 2012 at 10:15

GoogleCodeExporter commented 8 years ago
Thanks but this has already been fixed. The first cut of the code actually used 
a ConcurrentDictionary but was changed during performance profiling and after 
reviewing locking at the container level. It did not occur to me that two 
containers would be used within the same AppDomain.

Original comment by alex.meyergleaves on 9 Jun 2012 at 10:36

GoogleCodeExporter commented 8 years ago
Upon reflection keeping it as static seems wrong to me. It should be scoped to 
the container it belongs to. Especially since it's part of a "product" that is 
so concerned with lifecycle management, an example should be set. It may sound 
like nitpicking, and yes the issue is resolved, but I think this is an 
important design issue that deserves consideration. Feel free to ignore this 
comment. Just an observation that does not diminish your hard work which is 
very much appreciated.

Original comment by yves.rey...@gmail.com on 9 Jun 2012 at 12:31

GoogleCodeExporter commented 8 years ago
Your observation is certainly valid and I agree that this would be better. I 
intend to do more work in this area but will probably leave that for the 3.0 
branch. It is simply a case of trying to keep the changes in this branch to a 
minimum.

Original comment by alex.meyergleaves on 9 Jun 2012 at 12:43