ChangeProxyTarget throws System.InvalidCastException

[yallie]

Hi, I use Castle.DynamicProxy with DryIoc container to make the dependencies lazy transparently.
Instead of a real dependency, the container injects a lazy proxy. It works like this.

1. Create the interface proxy with target interface and specify no target.
2. Attach an interceptor with the lazy-wrapped target.
3. Create the target on the first method invocation and patch the proxy:

public class LazyInterceptor<T> : IInterceptor
	where T : class
{
	public LazyInterceptor(Lazy<T> lazyTarget)
	{
		LazyTarget = lazyTarget;
	}

	private Lazy<T> LazyTarget { get; }

	public void Intercept(IInvocation invocation)
	{
		var target = invocation.InvocationTarget as T;
		if (target == null)
		{
			// create the lazy value on the first invocation
			var proxyTarget = LazyTarget.Value;
			(invocation as IChangeProxyTarget).ChangeInvocationTarget(proxyTarget);
			(invocation as IChangeProxyTarget).ChangeProxyTarget(proxyTarget);
		}

		invocation.Proceed();
	}
}

It works like a charm when used with non-generic interfaces.
When used with generic interfaces, it works only for the first concrete type
created out of each generic interface. For example, given ICommand<T> interface,
it would work for ICommand<string> and fail for any other ICommand<...>.

Here is the complete test case used to reproduce the bug:

using System;
using Castle.DynamicProxy;

class Program
{
	static void Main()
	{
		var generator = new ProxyGenerator();

		Console.WriteLine("\n============= EventArgs1 ============");
		var lazyTarget1 = new Lazy<IEventHandler<EventArgs1>>(() => new Handler1());
		var lazyInterceptor1 = new LazyInterceptor<IEventHandler<EventArgs1>>(lazyTarget1);
		var proxy1 = generator.CreateInterfaceProxyWithTargetInterface<IEventHandler<EventArgs1>>(null, new[] { lazyInterceptor1 });

		proxy1.Handle(EventArgs.Empty);

		Console.WriteLine("\n============= EventArgs2 ============");
		var lazyTarget2 = new Lazy<IEventHandler<EventArgs2>>(() => new Handler2());
		var lazyInterceptor2 = new LazyInterceptor<IEventHandler<EventArgs2>>(lazyTarget2);
		var proxy2 = generator.CreateInterfaceProxyWithTargetInterface<IEventHandler<EventArgs2>>(null, new[] { lazyInterceptor2 });

		proxy2.Handle(EventArgs.Empty);
	}
}

public class LazyInterceptor<T> : IInterceptor
	where T : class
{
	public LazyInterceptor(Lazy<T> lazyTarget)
	{
		LazyTarget = lazyTarget;
	}

	private Lazy<T> LazyTarget { get; }

	public void Intercept(IInvocation invocation)
	{
		var target = invocation.InvocationTarget as T;
		if (target == null)
		{
			// create the lazy value on the first invocation
			var proxyTarget = LazyTarget.Value;
			Console.WriteLine($"Created the target value on the first method call: {proxyTarget}");

			(invocation as IChangeProxyTarget).ChangeInvocationTarget(proxyTarget);
			Console.WriteLine($"Changed the current invocation target to {proxyTarget}");

			(invocation as IChangeProxyTarget).ChangeProxyTarget(proxyTarget);
			Console.WriteLine($"Changed the proxy target to {proxyTarget}");
		}

		invocation.Proceed();
	}
}

public interface IEventHandler
{
	void Handle(EventArgs ea);
}

public interface IEventHandler<T> : IEventHandler
	where T : EventArgs
{
}

public class EventArgs1 : EventArgs {}

public class Handler1 : IEventHandler<EventArgs1>
{
	public void Handle(EventArgs ea)
	{
		Console.WriteLine("Handler1.Handle(ea) is called");
	}
}

public class EventArgs2 : EventArgs {}

public class Handler2 : IEventHandler<EventArgs2>
{
	public void Handle(EventArgs ea)
	{
		Console.WriteLine("Handler2.Handle(ea) is called");
	}
}

This demo outputs the following:

============= EventArgs1 ============
Created the target value on the first method call: Handler1
Changed the current invocation target to Handler1
Changed the proxy target to Handler1
Handler1.Handle(ea) is called

============= EventArgs2 ============
Created the target value on the first method call: Handler2
Changed the current invocation target to Handler2

Unhandled exception: System.InvalidCastException: Unabled to cast object of type "Castle.Proxies.IEventHandler`1Proxy_1" to type "Castle.Proxies.IEventHandler`1Proxy".
   at Castle.Proxies.Invocations.IEventHandler_Handle.ChangeProxyTarget(Object )
   at LazyInterceptor`1.Intercept(IInvocation invocation)
   at Castle.DynamicProxy.AbstractInvocation.Proceed()
   at Program.Main()

Note that the first interface IEventHandler<EventArgs1> works fine.
The second interface IEventHandler<EventArgs2> fails on ChangeProxyTarget
method, but ChangeInvocationTarget method succeeds.

I'm going to create a pull request with the failing test case.

[jonorossi]

@yallie thanks for the very detailed bug report and unit test pull request.

Since you've got this far with a unit test, did you want to give it a try debugging InvocationTypeGenerator.ImplementChangeProxyTarget to determine why it is generating the wrong cast for the second proxy type.

https://github.com/castleproject/Core/blob/v4.1.1/src/Castle.Core/DynamicProxy/Generators/InvocationTypeGenerator.cs#L300-L314

[yallie]

Thanks @jonorossi for pointing me in the right direction.

Unfortunately I can't get the project to build in Visual Studio, can't run unit tests, etc.
Don't know the reason, but the Studio says «Build succeeded» but outputs no binaries
and doesn't detect any unit tests in the solution. Have you got any guidelines how to
build the project using Visual Studio 2017, what version is required, pre-requisites, etc?

For now I just used a simple text editor to add my unit test and built the code using build.cmd.
Verified that the first part of my unit test passes, and the second part fails, and that's it.

I tried creating a new project from existing code, but I see that there are lots of settings,
conditional compilation symbols, etc. used to build the library. I'm not sure I can replicate
these properly in a fresh empty project.

UPD. Nevermind, I re-created the project from scratch and got all tests visible
in the test explorer. At least now I can debug it :)

UPD2. Looks like VS2017 problems were due to lightweight solution load option
which is enabled by default. Disabling it solved the build problems.

[yallie]

When running my test, I see that methods ImplementChangeInvocationTarget
and ImplementChangeProxyTarget are executed only once during the unit test.
I assume this means that the invocation type is generated only once, because
it represents the method call defined in my base non-generic interface:

public interface IEventHandler
{
	string Handle(EventArgs ea);
}

But I think they should be generated twice, for IEventHandler<EventArgs1> and
IEventHandler<EventArgs2> because the invocation class must have different
target proxy type for each generic interface:

public class Handler1 : IEventHandler<EventArgs1> // target #1
{
	public string Handle(EventArgs ea) { ... }
}

public class Handler2 : IEventHandler<EventArgs2> // target #2
{
	public string Handle(EventArgs ea) { ... }
}

To me it looks like the problem in the InterfaceProxyTargetContributor class:

invocation = new CompositionInvocationTypeGenerator(
                    method.Method.DeclaringType,
                    method,
                    method.Method,
                    canChangeTarget,
                    null)
    .Generate(@class, options, namingScope).BuildType();

The invocation type generator is based on method.Method.DeclaringType,
which is base non-generic interface IEventHandler in my case.

But then it uses @class, which represents the proxy type for generic interface
IEventHandler<EventArgs1>. So the invocation type for base non-generic interface
ends up being bound with the proxy type for the concrete generic interface.

Not sure if it makes any sense and (if it does) how it can be fixed :)

Perhaps the CompositionInvocationTypeGenerator shouldn't use the target type
of method.Method.DeclaringType, but what should it use instead?

[yallie]

Here is the generated code in Reflector:

namespace Castle.Proxies.Invocations
{
    using Castle.DynamicProxy;
    using Castle.DynamicProxy.Internal;
    using Castle.DynamicProxy.Tests.BugsReported;
    using Castle.Proxies;
    using System;
    using System.Reflection;

    // single generated invocation type
    [Serializable]
    public class IEventHandler_Handle : CompositionInvocation, IChangeProxyTarget
    {
        public IEventHandler_Handle(IEventHandler handler1, object obj1, 
            IInterceptor[] interceptorArray1, MethodInfo info1, object[] objArray1)
            : base(handler1, obj1, interceptorArray1, info1, objArray1)
        {
        }

        public override void ChangeInvocationTarget(object obj1)
        {
            // everything is fine here
            base.target = (IEventHandler) obj1;
        }

        public override void ChangeProxyTarget(object obj1)
        {
            // and here is the problem: proxyObject is being cast to the first concrete proxy type
            // but the proxy can have another type, of course
            ((IEventHandler`1Proxy) base.proxyObject).__target = (IEventHandler<EventArgs1>) obj1;
        }

        public override void InvokeMethodOnTarget()
        {
            base.EnsureValidTarget(); // BTW, why duplicate method invocation here?
            base.EnsureValidTarget();
            string str = (base.target as IEventHandler).Handle((EventArgs) base.GetArgumentValue(0));
            base.ReturnValue = str;
        }
    }
}

[yallie]

Looks like it can be worked around like this:

public override void ChangeProxyTarget(object obj1)
{
        ((IProxyTargetAccessor)base.proxyObject).DynProxySetTarget(obj1);
}

provided that IProxyTargetAccessor gets a new method:

public interface IProxyTargetAccessor
{
        object DynProxyGetTarget();
        void DynProxySetTarget(object target); // we'll need to add this new method
        IInterceptor[] GetInterceptors();
}

[yallie]

My approach seems to work indeed, all tests now pass.
Here is the summary of my changes:

• Added IProxyTargetAccessor.DynProxySetTarget method
• Provided the implementation in ProxyInstanceContributor.ImplementProxyTargetAccessor
• For a proxy that doesn't support SetTarget, the method throws InvalidOperationException
• InvocationTypeGenerator calls DynProxySetTarget instead of setting the target field in the proxy class

So, the invocation type is now completely decoupled from the proxy type.
Other minor changes:

• Replaced the abstract method ProxyInstanceContributor.GetTargetReferenceExpression with GetTargetReference (the Reference is required for DynProxySetTarget implementation)
• Fixed the overridden methods in descendant classes as needed.

Hope that makes sense :)

Regards, Alex.

[jonorossi]

@yallie thanks for the pull request, I'll try to get to reviewing it this week.

To make a new release we really need to get the two issues assigned to the vNext milestone fixed so a release won't be available straight away.

[yallie]

Thanks a lot for your response!

So the release requirement is that at least two issues are fixed?
I can also fix an issue with the duplicate EnsureValidMethod invocation #295
in the generated InvokeMethodOnTarget code if it helps :)

Regards, Alex.

[jonorossi]

So the release requirement is that at least two issues are fixed?

No, there is already another fix pending release (https://github.com/castleproject/Core/blob/master/CHANGELOG.md).

We've got a couple of defects (https://github.com/castleproject/Core/milestone/4) one with AssemblyVersion and another with our package dependency versions which we really need to fix otherwise they'll just get worse the more releases we make, the AssemblyVersion one is a must it is a regression from our previous pre-.NET CLI build scripts.

[yallie]

Ah sorry, my bad!
Thanks for the explanation.

[mm3141]

We ran into this issue using WCF client proxies sharing a common interface and fixed it by adding base.interfaces to the cache key here, prohibiting the wrong sharing of the generated proxy:

Core/src/Castle.Core/DynamicProxy/Contributors/InterfaceProxyWithoutTargetContributor.cs

Lines 67 to 76 in 29e5eb2

	Type[] invocationInterfaces;
	if (canChangeTarget)
	{
	invocationInterfaces = new[] { typeof(IInvocation), typeof(IChangeProxyTarget) };
	}
	else
	{
	invocationInterfaces = new[] { typeof(IInvocation) };
	}
	var key = new CacheKey(method.Method, CompositionInvocationTypeGenerator.BaseType, invocationInterfaces, null);

[yallie]

@mm3141 I was hoping to find a simple solution like yours.
But your code doesn't seem to pass my unit test: Core293.cs.
Please try yourself.

[mm3141]

@yallie There are three occurences of the pattern in line 70 above in the contributors. Replacing all of these with this makes the test pass:
```
if (canChangeTarget)
{
invocationInterfaces = new Type[interfaces.Count + 2];
invocationInterfaces[0] = typeof(IInvocation);
invocationInterfaces[1] = typeof(IChangeProxyTarget);
interfaces.CopyTo(invocationInterfaces, 2);
}

[yallie]

I see, that's great! Thank you.

I like that it's a relatively small change compared to my PR.

But looks like this way an application ends up
generating a new invocation class for each interface, right?

ISomeWrapper<MyClass1>.MyMethod(); // generates Invocation #1
ISomeWrapper<MyClass2>.MyMethod(); // ...Invocation #2
ISomeWrapper<MyClass3>.MyMethod(); // ...Invocation #3

I'd rather share generated classes when it's possible because
it's much cheaper than generating them (less CPU, less memory).

My approach decouples the invocation from the target proxy class,
so it enables sharing the invocation classes.

[mm3141]

Yes, we don't have this many different proxies, so the performance impact is low, and we wanted a low-risk temporary fix for this.

[jonorossi]

@yallie I'm sorry for taking my sweet bloody time to get to this issue, I looked at it very briefly a couple of times but something didn't seem right and I didn't have the time to dig into it until now.

Many thanks for getting to the bottom of the defect and describing the defect and fix so well.

I realised what didn't make sense to me, that is IChangeProxyTarget.ChangeProxyTarget() completely, why would you ask the invocation to change your target when its job is to handle the invocation.

// Makes sense since you are changing the invocation's target
var proxyTarget = LazyTarget.Value;
(invocation as IChangeProxyTarget).ChangeInvocationTarget(proxyTarget);

// Seems strange, why not go directly to the proxy, it doesn't even set the invocation's target
(invocation as IChangeProxyTarget).ChangeProxyTarget(proxyTarget);
(invocation.Proxy as IProxyTargetAccessor).DynProxySetTarget(proxyTarget); // <-- like this

If I'm not missing anything I think we should mark ChangeProxyTarget obsolete (but keep your fix) and refer people to use the new DynProxySetTarget method, this decouples the proxy from the invocation types even more and makes the intent of the code clearer.

[yallie]

@jonorossi Thanks for the review! I've marked ChangeProxyTarget as obsolete and done all requested changes. Hope it's all OK now.

[jonorossi]

@yallie thanks, looking now. Do you agree with my thoughts on ChangeProxyTarget, you didn't mention if you agreed.

[yallie]

I agree that asking invocation.Proxy to change its target makes the intent of the code clearer. So I just marked the ChangeProxyTarget method as obsolete, like you suggested. The only thing that looks a bit out of sync to me is that the interface name is still IChangeProxyTarget, but renaming an interface would break many things :)

[jonorossi]

The only thing that looks a bit out of sync to me is that the interface name is still IChangeProxyTarget, but renaming an interface would break many things :)

I thought the same thing. IProxyTargetAccessor also indicates it is an accessor not a mutator too 😉 .

Just added a couple of small comments, do you mind squishing your commits after that. Thanks.

[yallie]

Concerning interface names — perhaps we could add new properly named interfaces and mark old versions as obsolete to get rid of them in the next major version.

do you mind squishing your commits after that

I don't mind at all, but could you please just check "squish commits" flag when merging my PR using the Github UI. Not sure if I can do it properly using plain git, sorry.

[jonorossi]

Concerning interface names — perhaps we could add new properly named interfaces and mark old versions as obsolete to get rid of them in the next major version.

Maybe we don't even need an interface for invocations, just add a setter to InvocationTarget and throw in the override for class/inheritance proxies? Did you have any suggestions for a name for the proxy internals one?

ld you please just check "squish commits" flag when merging my PR using the Github UI

Yep, will do.

[yallie]

Maybe we don't even need an interface for invocations, just add a setter to InvocationTarget and throw in the override for class/inheritance proxies?

Yes, I'd prefer this approach over interfaces. Less type casts, less things to remember, and less code generation. The only downside I see is that interfaces allow checking explicitly whether an operation is supported. Not sure if someone actually uses it, though.

Did you have any suggestions for a name for the proxy internals one?

I'm not a native speaker, and I don't quite see why IProxyTargetAccessor is so bad :)
I think of it as a way to access the proxy internals.

[jonorossi]

I've merged the pull request without additional changes.

• I agree there is no need to make a new name for IProxyTargetAccessor.
• IChangeProxyTarget also does the job without any breaking changes, we can look at adding a setter to IInvocation in the future if we decide that would be a better way, but as you said the interface makes it really easy to know if you are allowed to change the invocation target before calling it.

Thanks for the changes. Fixed by #294.