castleproject / Core

Castle Core, including Castle DynamicProxy, Logging Services and DictionaryAdapter
http://www.castleproject.org/
Other
2.2k stars 467 forks source link

DI behavior change between .NET 7.0 and .NET 8.0 #678

Closed snowinmars-debug closed 1 week ago

snowinmars-debug commented 2 months ago

There is a breaking change in net7 -> net8 migration. I have a repo, that works on net7 + castle 4.4.1 and does not work on net8 + castle 4.4.1. It does not work on Castle 5.x either.

I made an issue in dotnet repository, but maybe I should write here.

All the details available by links.

Is it possible, that the root of the issue is in Castle library?

snowinmars-debug commented 2 months ago

I made a little research. It seems that Castle does not initialize DI properly.

If I add the following code in a repo above, the DI passes without the error: services.AddScoped<Castle.DynamicProxy.IInterceptor[], TestInterceptor[]>((a) => []);

It is just a moq code to test the root of the issue. If I'll get, how to initialize IInterceptor[] properly, the bug will be fixed.

jonorossi commented 2 months ago

Castle DynamicProxy does not use Microsoft.Extensions.DependencyInjection. Your repro has both Microsoft.Extensions.DependencyInjection and your own reflection emit code, you'll have to provide a repro using just Castle Core code for anyone to investigate a potential bug in DynamicProxy.

Closing as won't fix until the repro is reduced.

snowinmars-debug commented 1 month ago

That's not a potential bug, that's just a bug) Dotnet devs says the same.

The correct way to fix it is to add [Optional, DefaultParameterValue(null)] on the ctor argument.

I russian it up by the following:

private static IServiceCollection Foo(this IServiceCollection serviceCollection)
{
    var already = serviceCollection.Any(x => x.ServiceType == typeof(IInterceptor[]));
    if (!already) serviceCollection.AddSingleton<IInterceptor[]>(x => null);
    return serviceCollection;
}

// ...

return serviceCollection
    .Foo()
    .AddTransient(typeof(RuntimeInterceptorSelector<>).MakeGenericType(implementationType))
    .AddSingleton(accessorType, Activator.CreateInstance(accessorType, (object)interceptorTypes));
stakx commented 2 weeks ago

Quick update: I ran our unit test suite for .NET 6, 7, and 8, which produces no failures... so apparently this is a usage scenario that our test suite does not yet cover.

The repo provided by @snowinmars-debug uses DynamicProxy v4 and includes code that meddles with DynamicProxy internals, which we've since removed from the public API... so basically the current repro does things that we've chosen no longer to support (as already mentioned above).

@snowinmars-debug, any chance you could provide repro code for DynamicProxy v5?

stakx commented 2 weeks ago

@snowinmars-debug, upon taking a closer look at your code, I think you could solve your problem simply by letting DynamicProxy's own ProxyGenerator handle the proxy type instantiation, instead of fiddling around with DynamicProxy internals yourself and leaving the proxy type instantiation up to the DI container. This should be easily possible by using a factory function in your ServiceDescriptor(s)... something like this perhaps:

+        private static ProxyGenerator generator = new();

         private static IServiceCollection AddWithInterceptors(
             this IServiceCollection serviceCollection,
             Type serviceType,
             Type implementationType,
             Type[] interceptorTypes,
             ServiceLifetime serviceLifetime)
         {
-            var withTargetGenerator = new TypedInterfaceProxyWithTargetGenerator(serviceType, implementationType);
-            var proxiedImplementationType = withTargetGenerator.GenerateCode(implementationType, Type.EmptyTypes, ProxyGenerationOptions.Default);

             serviceCollection.Add(new ServiceDescriptor(implementationType, implementationType, serviceLifetime));
-            serviceCollection.Add(new ServiceDescriptor(serviceType, proxiedImplementationType, serviceLifetime));
+            serviceCollection.Add(new ServiceDescriptor(serviceType, serviceProvider =>
+            {
+                var proxy = generator.CreateInterfaceProxyWithTarget(
+                    interfaceToProxy: serviceType,
+                    target: serviceProvider.GetService(implementationType),
+                    interceptors: interceptorTypes.Select(serviceProvider.GetService).Cast<IInterceptor>().ToArray());
+                return proxy;
+            }, serviceLifetime));

             return serviceCollection
                 .AddInterceptorsForType(interceptorTypes, implementationType);
         }

This appears to work just fine, and it also appears to circumvent that breaking change regarding HasDefault.

snowinmars-debug commented 1 week ago

@snowinmars-debug, any chance you could provide repro code for DynamicProxy v5?

Already done: swap these lines. The commented ones are for v5.

I should've left a comment there about it)

stakx commented 1 week ago

@snowinmars-debug, sorry but that doesn't seem plausible: the commented-out lines of code depend on a class whose base class is no longer exposed by DynamicProxy v5. Your code shouldn't compile at all with v5.

Either way, I think a repro is no longer needed since the reported problem goes away once you go through DynamicProxy's public API instead of using its internals (see my last post above). Unless that approach doesn't work at all for you, I'm going to close this issue in a few days' time.