Castle.Windsor.Extensions.DependencyInjection parallelism issues #563
Comments
|
Hi @nativenolde I will look into it. Of the top of my head I expect it has to do with the scope handling being a static. Your tests are great, give me a short time I will get back o you... |
|
Sure, take your time ... i guess it's getting complicated ;) |
|
@nativenolde is it just me, or is nothing attached? You can also expose a repo if you prefer. |
|
|
|
OMG. noob here. Only saw 1 hyperlink. thanks. |
|
Hi @nativenolde I changed the test framework to Nunit. From stack overflow: NUnit creates a new instance of the test class and then runs all of the tests method from the same instance. And each test fixture i believe is its own context. NUnit fixtures runs more like the host would. and with same memory.
Please review my conclusion. I will try to see if i can get the scope not to be static. But a since factory with a single host should not have any issues You see this also passes:
|
|
You see, here is the issue we need to improve to make this no longer an issue for test: References a static:, and i cannot get the container or resolve the scope in any other way.. Until we get rid of the static scope pointers, there can only be 1 pr assembly. All Castle.Windsor scops use a static memory for allocation.. |
|
Yes, you are right, a single instance with a huge amount of parallel requests seems to work. As windsor addresses enterprise and advanced needs, this could become a problem, because those users probably have a DevOps (ci/cd) workflow that could include such parallel tests. Do you see a possibility to get the this redesigned or workaround? |
|
@nativenolde i looked into it last night. I would need to rewrite the scope handling in Windsor not to utilize a static initialization. I am not saying it is undoable, I just need to think more ;-) |
|
many thanks in advance for your effort! |
|
@nativenolde @AntonioDrusin @robertcoltheart @twenzel @ltines I think i might be able to extend the IScopeAccessor to allow for some kernel resolving. The resolving getting a scope cache instance..
if (accessor.HasKernelScoping)
{
**var container = Kernel.Resolve<IScopeContainer>();
scope = localScope.GetScope(context, Kernel);**
}
else
{
scope = localScope.GetScope(context);
}then the ExtensionContainerScope can be an instance instead of a static. public ILifetimeScope GetScope(CreationContext context, IKernel kernel)
{
**var current = kernel.Resolve<IExtensionContainerScope>();**
if(ExtensionContainerScope.Current == null)
{
throw new InvalidOperationException("No scope available");
}
return current.Current1;
}Do I have any community support to follow through on this train of thought? NB the committed code needs more changes to work, and I do have tests passing on my end. But you would need to do more to test yourself. The PR was onlky ment at as a item for discussion.,.. /cc @jonorossi |
|
@generik0 I've left a few comments on #567, the direction you have taken isn't how this should work. Scope accessors usually use some external context to know what "scope" you are in, it doesn't make sense creating a scope object to stick it into the container just to resolve it back out inside a scope accessor. I'm unsure if resolving inside a scope accessor is even a good idea, there may be reentrancy issues. Scopes by definition are tied to a single container, it is the external context that needs to be sorted (e.g. like our ASP.NET scope used to use the static |
|
Hi @jonorossi Personally l, one of the things I dislike about aspnet(core) the most is the static HttpContext. Just like the static implementation of scope in castle. Something should own the Scopes, and be able to access them. I have looked at many other DIs, her the container is the owner of the scope, it does not appear to be static. If we do not get ride of the static scopes, we will not be able to run more than 1 provider in the same assembly as this issue highlights. But we can also decide to leave it be, and say it is by design. |
|
xUnit Workaround: Turn off parallelism inside the assembly |
|
The problem comes from the fact the code expectes there will be only one host per app domain / running process. I think that would solve your issue. But I am thinking how relevant is it to real world use cases? |
|
@ltines I think it's a relevant issue in the context of unit tests: in the projects I work on, we also typically run the unit tests in parallel and have a separate castle windsor container setup for each test. It would be limiting if that would not be possible. Is this related to the static |
|
@rvdginste it is related to ExtensionContainerRootScope and ExtensionContainerScope but primarily the GetScope method in the scope accessor, using the static scopes. @ltines how would you bind to IHost If it is static? It isnt just s matter of the uTests i feel. It is also an issue since the MS DI and other DIs do not fail on this. Hence, our implementation is not 100% conforming. And my point is also that static scope handling is not stateless static. |
|
@jonorossi @rvdginste @ltines @nativenolde @AntonioDrusin please have a look at PR #573 Have a look, please have a look if/when you have time-> Is this the correct path? Please note, yes it needs to be cleand up... Please look at the feeling behind not the change. Not my mess. |
|
@jonorossi @rvdginste @ltines @nativenolde @AntonioDrusin please have a look at PR #573 Anyone have feelings about my cleanup of the assembly? Managed to get rid of most of the static "stuff in favour of a singleton. However, the root must be a asynlocal to allow for multiple for the Parallelization of this issue.. I only have next week (week 49) to finish this up before the next job.. If anyone has thoughts / time, It should be great! My feeling is, This cleans up the lib and makes it more tracable / readable .. Statics removed in favor of singleton instance of ExtensionContainerScope. |
|
Pull request #573. |
|
@generik0 @ltines @jonorossi @nativenolde @generik0 I checked your code, but I had the impression that @jonorossi recommended to not register the root scope in the container. Either way, your code gave me an idea and I made a pull request myself: #577 . The changes are pretty small and are based on the following:
The existing unit tests run fine and the test project from @nativenolde runs fine with these changes. Still I'm not confident that the code does not break anything: I'm not familiar with the inner workings of Castle Windsor. Please take a look. |
|
@rvdginste Yes, your change is more simple/clean. I have closed my PR. |
|
Thank you guys for fixing that! |
|
@jonorossi would it be possible to release a new nuget version for this? if i'm correct the latest release is 5.1.1 released on dec 8, but this fix was merged on dec 16. |
To fix issue castleproject#563 (support concurrently existing containers) a fix was created in castleproject#577. After the initial fix some refactorings were done on the scope implementation that were not correct. This commit changes the scope implementation back to the original implementation of @ltines, but with the support for concurrent containers.
To fix issue castleproject#563 (support concurrently existing containers) a fix was created in castleproject#577. After the initial fix some refactorings were done on the scope implementation that were not correct. This commit changes the scope implementation back to the original implementation of @ltines, but with the support for concurrent containers.
To fix issue castleproject#563 (support concurrently existing containers) a fix was created in castleproject#577. After the initial fix some refactorings were done on the scope implementation that were not correct. This commit changes the scope implementation back to the original implementation of @ltines, but with the support for concurrent containers.
@ltines @generik0 thank you for contributing the Castle.Windsor.Extensions.DependencyInjection package.
I guess a lot of people are very happy about that!
Since migrating my application to use the ms-di implementation (v5.1), i've noticed some potentially serious issues. They occur when running multiple ASP.NET Core Integration Tests in parallel on independent application instances. It seems some dependencies will be shared respectively disposed across HTTP-Request boundaries. At the moment, i can't say whether this is just a problem when testing or it's affecting production runtime as well.
My reproducible sample, created using Microsoft guidelines
https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests
is attached here
InteractionTesting.zip
As long as the tests are executed serial they work properly.. but if you run them in parallel, they fail for different reasons.
As an example:
If you stick with the built-in service provider and do not use windsor, it works fine even if the tests will be executed in parallel.
Would you be so kind and take a look at it?
The text was updated successfully, but these errors were encountered: