-
Notifications
You must be signed in to change notification settings - Fork 457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Castle.Windsor.Extensions.DependencyInjection to support .NET8 #668
base: master
Are you sure you want to change the base?
Conversation
…removed null check on scope cache (AsyncLocal can be null on Threads coming from Threadpool.UnsafeQueueUserWorkItem, having no null check was also the original behavior)
Threadpool.UnsafeQueueUserWorkItem.
this improve resolution of singleton object that might not need a root scope, the windsor container should be enough to resolve singletons
scopes from Threadpool.UnsafeQueueUserWorkItem
Major fix was to support keyed registration scenario
The previous handling of root scope is wrong, the code set root scope in an AsyncLocal variable when WindsorServiceProviderFactoryBase was first creatd. The problem arise with kestrel or orleans in .NET 8, they use a Thread Pool that runs code outside AsyncLocal so it will break resolution. It was not possible to reproduce locally, but it was reproduced with production code. A repro for the bug still missing. Also we can support using a Global root scope only if we use only ONE CONTAINER in the .NET core DI, because basic structure does not allow to find the container that is resolving scoped component thus we cannot determin the right root context. Still work to do to support multiple container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some cleanup to do!
@@ -23,7 +23,7 @@ | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(TargetFramework)'=='net6.0'"> | |||
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="6.0.0" /> | |||
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="8.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should support proper multitarget, we decided that only net8.0 should have 8.x references (see Castle.Windsor.Extensions.DependencyInjection)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
buildscripts/common.props
Outdated
@@ -18,7 +18,7 @@ | |||
<Product>Castle Windsor</Product> | |||
<FileVersion>$(BuildVersionNoSuffix)</FileVersion> | |||
<VersionPrefix>$(BuildVersion)</VersionPrefix> | |||
<AssemblyVersion>$(BuildVersionMajor).0.0</AssemblyVersion> | |||
<AssemblyVersion>6.0.0.0</AssemblyVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this file to original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
@@ -25,9 +25,14 @@ internal abstract class ExtensionContainerScopeBase : ILifetimeScope | |||
public static readonly string TransientMarker = "Transient"; | |||
private readonly IScopeCache scopeCache; | |||
|
|||
private static long _counter; | |||
|
|||
public long Counter { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was added for debugging purposes, we can get rid of this counter
@@ -138,6 +138,10 @@ protected virtual object CreateInstance(CreationContext context, ConstructorCand | |||
protected object CreateInstanceCore(ConstructorCandidate constructor, object[] arguments, Type implType) | |||
{ | |||
object instance; | |||
if (Model.Implementation.FullName.Contains("SiloConnectionFactory")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Seems there is related issue |
Needed to modify basic Castle.Winsor library to support the ability from IHandler interface to get the current kernel associated with the handler. This is needed to find the correct root scope associated with that kernel instance.
The project uses Windsor 6.0, and after configuring orleans client 8.0, it prompts "This service descriptor is keyed." Your service provider may not support key services |
This pull request introduced keyed registration only in .NET 8, please confirm that your project targets .NET 8. |
Lots of time passed, is there anyone that can comment/reject/accept this Pull Request? |
Yeah, what's taking so long? We're running into the same issues here, this PR fixes that.. |
It would be great to get this support in Castle Windsor. I did notice that this PR has lots of checks like |
From what I know, keyed service provider was introduced in ASP.NET core in .NET 8 and it is not present in previous version. I can be wrong but I was forced to add that directive because code does not compile in .NET 6 (have not tried with 7) |
It was released along with .NET 8, but doesn't require .NET 8. Microsoft.Extensions.DependencyInjection 8.0.0 is supported on multiple frameworks. I've used MS DI with keyed services on .NET Framework just fine. You should just need to update the NuGet package to 8.0.0 in your .NET 6 project and it should compile. |
Perfect, I'll modify the branch as soon as possible. This will force people using castle on .NET 6 to update to version 8 of the DI package but I think that this can be just fine. |
The bug happened when you register a service with one NON keyed component then again with KEYED components. The adapter incorrectly checked only the first returned service for KEYED and returns null. Identified after update to Orleans 8.1.0
If someone used the version locally compiled, I've pushed a fix for a bug that makes impossible to use Microsoft Orleans 8.1.0, the error derived on how the basic Microsoft Dependency injection works with multiple registration of the same NON keyed service. Update the test suite, tests are now green, Microsoft.Orleans 8.1.0 works. |
@jklawrence I do not know if any maintainer is checking this PR. I can do the modification you requested, but this will force people using version 8.0.0 of dependency injection even if they use .NET 6 framework. I do not see any problem but I'd like some maintainer have last word on it. |
This Pull Request contains also all modification by Alessandro Giorgetti already made in PR #664 related to issue #646
This Pull Request aim to give Castle.Windsor.Extensions.DependencyInjection full support to .NET 8 new DI model with keyed service. The reason of this Fork was the inability to upgrade ours production code to .NET 8 due to internal error in Castle.Windsor. This problem happens especially with Microsoft Orleans 8 and sometimes code running on Kestrel 8.
We actually start using this code in our production code, because without these modification we have as only option to use Orleans 8 to entirely remove Castle.Windsor from our project (An extemely big work) so this code, starting from today, is running in our solution.
Feel free to ask any information or require modification in code if you feel that something is wrong.
Thanks.