Skip to content
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

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

Closed
snowinmars-debug opened this issue Jul 3, 2024 · 7 comments
Closed

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

snowinmars-debug opened this issue Jul 3, 2024 · 7 comments

Comments

@snowinmars-debug
Copy link

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@jonorossi jonorossi reopened this Aug 12, 2024
@stakx
Copy link
Member

stakx commented Aug 29, 2024

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
Copy link
Member

stakx commented Aug 29, 2024

@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
Copy link
Author

@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
Copy link
Member

stakx commented Sep 2, 2024

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

@stakx stakx removed the needs-repro label Sep 2, 2024
@stakx stakx closed this as completed Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants