Update Castle.Windsor.Extensions.DependencyInjection to support .NET8 #668

alkampfergit · 2024-02-15T14:35:03Z

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.

Modified code to support KeyedService in .NET 8. All basic test from framework passes, also this pr contains some other tests that idenfy some problem that occurred in our production code
Scope management: Orleans 8 changed how it resolves grains so we always got a null Root Scope. This happens because the Root Scope is marked with AsyncLocal, but Orleans uses thread differently so that AsyncLocal is always null.
We introduced a modification in base Castle.Windsor to allow for IHandler to return the current kernel object. While this is somewhat an Hack, it was the only way for us to find the correct root scope when that AsyncLocal is null. This Hack can be avoided if we always use one single instance container, but this is a too restrictive situation.

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.

…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.

files

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

deploy packages

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.

AGiorgetti

There is some cleanup to do!

AGiorgetti · 2024-02-15T17:24:29Z

src/Castle.Windsor.Extensions.Hosting/Castle.Windsor.Extensions.Hosting.csproj

@@ -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" />


should support proper multitarget, we decided that only net8.0 should have 8.x references (see Castle.Windsor.Extensions.DependencyInjection)

AGiorgetti · 2024-02-15T17:25:47Z

buildscripts/common.props

@@ -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>


revert this file to original

AGiorgetti · 2024-02-15T17:31:15Z

src/Castle.Windsor.Extensions.DependencyInjection/Scope/ExtensionContainerScopeBase.cs

@@ -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; }


was added for debugging purposes, we can get rid of this counter

AGiorgetti · 2024-02-15T17:32:47Z

src/Castle.Windsor/MicroKernel/ComponentActivator/DefaultComponentActivator.cs

@@ -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")) 


remove this code

AlexanderKot · 2024-02-17T13:47:35Z

Seems there is related issue
Finbuckle/Finbuckle.MultiTenant#786
Finbuckle multi tenancy + CW as DI container (initially 5.1.2, migration to 6.0.0 do not help)

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.

bxjg1987 · 2024-03-02T17:28:20Z

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

alkampfergit · 2024-03-03T06:30:35Z

This pull request introduced keyed registration only in .NET 8, please confirm that your project targets .NET 8.

alkampfergit · 2024-03-08T14:45:50Z

Lots of time passed, is there anyone that can comment/reject/accept this Pull Request?

TheDutchDev · 2024-03-28T08:58:52Z

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..

jklawrence · 2024-05-06T14:33:41Z

It would be great to get this support in Castle Windsor.

I did notice that this PR has lots of checks like #if NET8_0_OR_GREATER, but I think those could simply be removed. Keyed services are supported in .NET 6.0, 7.0, and even .NET Framework - anywhere Microsoft.Extensions.DependencyInjection is supported.

alkampfergit · 2024-05-06T16:10:59Z

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)

jklawrence · 2024-05-07T18:45:31Z

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.

alkampfergit · 2024-05-08T06:18:16Z

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

alkampfergit · 2024-05-22T09:40:08Z

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.

alkampfergit · 2024-05-22T09:42:04Z

@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.

If multiple concrete classes are registered in castle, when you resolve castle resolves the first one. With Microsoft DI is the oposite, you want the last registered. The resolution is now fixed to honor IsDefault() because previous code registered every component with IsDefault() if it is registered from the adapter.

alkampfergit · 2024-06-24T15:47:35Z

If you are interested, I've updated the PR adding a fix that correctly restore the correct use of IsDefault(). In previous version of the adapter all services registered through the adapter with Microsoft DI services registered componente with the IsDefault() (Due to a different rule in resolution). That actually make not possible to correctly use the IsDefault() anymore. Fixed.

We added a new concept, an extendede property that allows the code to understand if the dependency was registered through the adapter (ServiceCollection) or directly through the Container. This allows us to change the resolution rule in case of multiple services registered with the same name.

jonorossi

Can you clear out all the other code changes that are not part of this change and look at my comments.

jonorossi · 2024-07-11T09:14:55Z

src/Castle.Windsor/MicroKernel/IHandler.cs

+		/// Needed to support root scope for multiple container when integrating with .NET 8
+		/// DI engine.
+		/// </summary>
+		IKernel GetKernel();


This is a fairly severe breaking change to IHandler, need to think about this more.

jonorossi · 2024-07-11T09:19:27Z

.config/GitVersion.yml

+ignore:
+  sha: []
+merge-message-formats: {}
+mode: ContinuousDeployment


Let's keep the pull request to just adding support for .NET 8. The diff is 2600+ LOCs.

jonorossi · 2024-07-11T09:21:55Z

...ensions.DependencyInjection.Tests/Castle.Windsor.Extensions.DependencyInjection.Tests.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFrameworks>netcoreapp3.1;net6.0</TargetFrameworks>
+    <TargetFrameworks>netcoreapp3.1;net6.0;net8.0</TargetFrameworks>


Others have said we can add support for keyed services without adding another compile target, that sounds like a good idea.

Windsor should then support .NET 6 until at least end of support (November 12, 2024) and then upgrade to the next LTS.

jonorossi · 2024-07-11T09:22:13Z

...ensions.DependencyInjection.Tests/WindsorScopedServiceProviderCustomWindsorContainerTests.cs

+
+		//	// Assert
+		//	Assert.IsType<FakeService>(service);
+		//}


Commented out code

...ensions.DependencyInjection.Tests/WindsorScopedServiceProviderCustomWindsorContainerTests.cs

jonorossi · 2024-07-11T09:26:51Z

src/Castle.Windsor.Extensions.DependencyInjection/Extensions/WindsorExtensions.cs

+			// I don't think we need this lifestyle at all, usual Singleton should be good enough;
+			// also we maybe don't need the whole rootscope thing. A normal scope set as current should be enough
+			// otherwise we should revert to static rootscope


TODO comments

jonorossi · 2024-07-11T09:27:57Z

src/Castle.Windsor.Extensions.DependencyInjection/RegistrationAdapter.cs

-			return registration.UsingFactoryMethod((kernel) => {
+			return registration.UsingFactoryMethod((kernel) =>
+			{
+				//TODO: We should register a factory in castle that in turns call the implementation factory??


TODO comments

jonorossi · 2024-07-11T09:29:26Z

src/Castle.Windsor.Extensions.DependencyInjection/Scope/ExtensionContainerScope.cs

+using System;
+


Check out the Castle coding style conventions, they may be a bit strange and different to what is common today, but if we follow them it keeps things more readable and maintainable.

jonorossi · 2024-07-11T09:29:39Z

src/Castle.Windsor.Extensions.DependencyInjection/Scope/ExtensionContainerScopeCache.cs

 		/// <summary>Current scope for the thread. Initial scope will be set when calling BeginRootScope from a ExtensionContainerRootScope instance.</summary>
 		/// <exception cref="InvalidOperationException">Thrown when there is no scope available.</exception>
 		internal static ExtensionContainerScopeBase Current
 		{
-			get => current.Value ?? throw new InvalidOperationException("No scope available");
+			// AysncLocal can be null in some cases (like Threadpool.UnsafeQueueUserWorkItem)
+			get => current.Value; // ?? throw new InvalidOperationException("No scope available. Did you forget to call IServiceScopeFactory.CreateScope()?");


Commented out code

jonorossi · 2024-07-11T09:30:50Z

src/Castle.Windsor.Extensions.DependencyInjection/WindsorScopedServiceProvider.cs

+			// root scope should be tied to the root IserviceProvider, so
+			// it has to be disposed with the IserviceProvider to which is tied to
 			if (!(scope is ExtensionContainerRootScope)) return;
 			if (disposing) return;
 			disposing = true;
 			var disposableScope = scope as IDisposable;
 			disposableScope?.Dispose();
+			// disping the container here is questionable... what if I want to create another IServiceProvider form the factory?


What are these comments for?

AGiorgetti and others added 14 commits February 9, 2024 14:21

Refs: castleproject#646 Castle.Windsor.Extension.DepencencyInjection …

08ad3dc

…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)

ExtensionContainerRootsScopeAccessor might return a null root scope in

a8347ea

Threadpool.UnsafeQueueUserWorkItem.

Added More failing test cases

ba03d1b

Added Threadpool.Unsafe... failing tests, added comments on critical

df9dd36

files

Improved tests and comments

64df1b0

Option to map custom NetStatic to Castle Singleton lifestyle.

28ac185

this improve resolution of singleton object that might not need a root scope, the windsor container should be enough to resolve singletons

RootScope AsyncLocal cache can be null when AspNetCore tries to create

a210c2a

scopes from Threadpool.UnsafeQueueUserWorkItem

Improved Tests showing supported, unsupported and memory leaks scenarios

b390405

Added comments on WindsorScopedServiceProvider dispose

b997da0

Updated Castle.Windsor reference to nuget package to create private

5532539

deploy packages

Added NuGet package build instructions

518cc84

added gitversion

81c6b05

Update to work with .NET 8 dependency registration.

3707b52

Major fix was to support keyed registration scenario

AGiorgetti mentioned this pull request Feb 15, 2024

Refs: #646 Castle.Windsor.Extension.DepencencyInjection - No Scope Available #664

Closed

AGiorgetti reviewed Feb 15, 2024

View reviewed changes

jonorossi mentioned this pull request Feb 18, 2024

NET 6.0 ForcedScope Failed with "No Available Scope" on GET request #646

Open

alkampfergit force-pushed the master branch from c492671 to 6a2c016 Compare February 18, 2024 10:29

Added logic to support multiple container in .NET 8 DI

7437a7c

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.

alkampfergit force-pushed the master branch from 6a2c016 to 7437a7c Compare February 18, 2024 10:30

Changed constants in keyedregistration helper.

36bbbf1

maliming mentioned this pull request Mar 28, 2024

Runtime error when using the new .NET 8 Keyed Services in ASP.NET Boilerplate volosoft/castle-windsor-ms-adapter#48

Open

alkampfergit force-pushed the master branch from 8529c4f to 17d1010 Compare May 22, 2024 09:18

alkampfergit mentioned this pull request May 22, 2024

Use Root scope when current is null (fixes #646) #670

Open

alkampfergit force-pushed the master branch from 6adaf28 to 9499d3a Compare June 24, 2024 15:36

alkampfergit force-pushed the master branch 2 times, most recently from d2042c2 to b71829c Compare June 25, 2024 15:27

jonorossi requested changes Jul 11, 2024

View reviewed changes

Cleanup from PR Requests

495fc21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Castle.Windsor.Extensions.DependencyInjection to support .NET8 #668

Update Castle.Windsor.Extensions.DependencyInjection to support .NET8 #668

alkampfergit commented Feb 15, 2024 •

edited

Loading

AGiorgetti left a comment

AGiorgetti Feb 15, 2024

alkampfergit Feb 18, 2024

AGiorgetti Feb 15, 2024

alkampfergit Feb 18, 2024

AGiorgetti Feb 15, 2024

AGiorgetti Feb 15, 2024

alkampfergit Feb 18, 2024

AlexanderKot commented Feb 17, 2024

bxjg1987 commented Mar 2, 2024

alkampfergit commented Mar 3, 2024

alkampfergit commented Mar 8, 2024

TheDutchDev commented Mar 28, 2024

jklawrence commented May 6, 2024

alkampfergit commented May 6, 2024

jklawrence commented May 7, 2024

alkampfergit commented May 8, 2024

alkampfergit commented May 22, 2024 •

edited

Loading

alkampfergit commented May 22, 2024

alkampfergit commented Jun 24, 2024

jonorossi left a comment

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

jonorossi Jul 11, 2024

Update Castle.Windsor.Extensions.DependencyInjection to support .NET8 #668

Are you sure you want to change the base?

Update Castle.Windsor.Extensions.DependencyInjection to support .NET8 #668

Conversation

alkampfergit commented Feb 15, 2024 • edited Loading

AGiorgetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderKot commented Feb 17, 2024

bxjg1987 commented Mar 2, 2024

alkampfergit commented Mar 3, 2024

alkampfergit commented Mar 8, 2024

TheDutchDev commented Mar 28, 2024

jklawrence commented May 6, 2024

alkampfergit commented May 6, 2024

jklawrence commented May 7, 2024

alkampfergit commented May 8, 2024

alkampfergit commented May 22, 2024 • edited Loading

alkampfergit commented May 22, 2024

alkampfergit commented Jun 24, 2024

jonorossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alkampfergit commented Feb 15, 2024 •

edited

Loading

alkampfergit commented May 22, 2024 •

edited

Loading