-
Notifications
You must be signed in to change notification settings - Fork 466
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
Add ability to mix in delegate (types) to proxies #436
Conversation
src/Castle.Core/DynamicProxy/Contributors/DelegateTypeMembersCollector.cs
Outdated
Show resolved
Hide resolved
Hi @stakx! The way you decided to sort it out looks promising and elegant! I’ll definitely give it a more precise look. In the meanwhile, I want to clarify whether the following feature is supported (found the concern in my old comment):
Thanks! :) |
@zvirja, nice to hear from you again!
I can't remember all the details from the previous issue, but basically this PR follows @jonorossi's proposal made in #403 (comment), where he also wrote:
That pretty much sums it up. DynamicProxy didn't have the ability to add custom attributes to single methods (AFAIK) and that hasn't changed. If that's needed, perhaps we'd need to look at it in a separate PR. |
This comment has been minimized.
This comment has been minimized.
fbb46e4
to
3467d89
Compare
@stakx Thanks for the replies. It started to ring a bell in my head now. Tried to give it a go, but it fails with the following error:
In our code I also mix in an additional object. It might be related to the finding you already described. Please ping me when it's ready, so I can give it a try 😉 From the first glance I could say that API looks perfect from the client perspective and should cover all the needs we have 👍 One more question (warning, might be stupid): are you sure that after all the changes you applied the container generated type is still cached? P.S. Here is my branch if you would like to take a look. |
*snip* (outdated bit that you can safely ignore)
I ran some quick tests this morning. Basically, caching should work as before if equality is correctly implemented on both |
@zvirja, there was a glaringly obvious bug that I didn't notice anymore at 2am last night. 😁 Fixing it also fixed all NSubstitute unit tests except one ( |
Speaking for Moq, I am very satisfied with how well this change would work out: stakx/moq@50fd9fa (14 insertions, 122 deletions). Almost all of the delegate special-casing code is gone, and gets replaced by the very short |
@stakx After working a bit more on our code I managed to finally get it work. Because The only issue I discovered is that delegate proxy activation degraded in terms of performance:
Obviously, I don't want you to profile our code 😅 But if you could benchmark Moq instead, it would be interesting to see whether you notice the same degradation. So it might be easier for you to check whether degradation resides somewhere in the code you are adding (as previously we also used |
@zvirja: I suspect that (Here are my quick benchmark results.)
(Not sure btw. why referencing Castle.Core as a project instead of its latest NuGet package makes such a big difference.) P.S.: Those additional ~2.5 μs/proxy likely won't matter much in practice. You'd have to create an awful lot of delegate proxies (say, 100,000 to 1 million) before that accumulates into a noticeable time span. I think it's a good price to pay if it means we can free our mocking libs from doing their own Refection.Emit-ing, and freeing Core to no longer expose its internal APIs ( |
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.
Looks great @stakx 🎉. I had to really review the thread where I proposed this design (forgot all the details), it really did work out well. Many thanks for getting this done.
68e8fef
to
2e0d8b2
Compare
This comment has been minimized.
This comment has been minimized.
If I'm not mistaken, the necessary changes for Windsor—excluding any possible optimizations—might look as follows (applied here): private Type GetProxyType(IProxyBuilder builder, Type targetDelegateType)
{
- var scope = builder.ModuleScope;
- var logger = builder.Logger;
- var generator = new DelegateProxyGenerator(scope, targetDelegateType)
- {
- Logger = logger
- };
- return generator.GetProxyType();
+ var options = new ProxyGenerationOptions();
+ options.AddDelegateTypeMixin(targetDelegateType);
+ return builder.CreateClassProxyType(typeof(object), Type.EmptyTypes, options);
} |
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's just one last change I'd like to make (turning TryCreateMixinToDelegate
into a throwing non-Try
version), then I think this will be ready to merge.
src/Castle.Core.Tests/DynamicProxy.Tests/DelegateMixinTestCase.cs
Outdated
Show resolved
Hide resolved
This adds the following capabilities to DynamicProxy: * `ProxyGenerationOptions.AddDelegateMixin` to add a delegate mixin. This will produce an `Invoke` method on the proxy that has the same signature as the delegate. The delegate can be `Proceed`-ed to. * `ProxyGenerationOptions.AddDelegateTypeMixin` adds an `Invoke` method on the proxy that has the same signature as a specified delegate type. When those `Invoke` methods are intercepted, they cannot be proceeded from as that mixin has no target. * `ProxyUtil.TryCreateDelegateToMixin` is a helper method to create a delegate of a given type to a suitable `Invoke` method on a proxy. By combining the above methods, you can now create a delegate proxy: var options = new ProxyGenerationOptions(); options.AddDelegateTypeMixin(typeof(Action)); var _ = generator.CreateClassProxy(typeof(object), options, ...); ProxyUtil.TryCreateDelegateToMixin(_, out Action proxy); ^^^^^^^^^^^^^^^^
Simply comparing the count of mixin types against a filtered distinct count triggers false positive conflict detection because the filter was only applied to one set of mixin types. Also, optimize the conflict detection logic & only run it when needed.
...and standardize on using `Delegate` instead of `MulticastDelegate` in public method signatures. (This might seem contradictory, since the delegate type check is based on the latter. But it will arguably cause users less confusion to see `Delegate` than the less familiar `Multi- castDelegate`. Also, BCL APIs such `Delegate.CreateDelegate` have a declared return type of `Delegate`. Let's do the same.)
From the user's point of view, there's no reason to expect `TryCreate- DelegateToMixin` to fail once a proxy with `options.AddDelegate[Type]- Mixin` has been created. If the method *does* fail, there will be no indication what might have gone wrong, and user code likely won't be able to proceed; so why not just let DynamicProxy throw an exception.
Yep, looks about right. Maybe we should mark
I agree, I'm glad you are changing it. I didn't comment on it but I did wonder what the use case of wanting to handle the failure case yourself was, just assumed you had a good reason 😉. |
Glad to hear you approve of that last-minute change, which didn't get reviewed. I was fairly confident that it was the right call, so I just went ahead. I've been a fan of the That, and I recently came to suspect that the |
* Windsor needs `TypeUtil.GetAllInterfaces`, so make it public again. * Windsor is currently relying on `DelegateProxyGenerator`, but there is now another, more public API to build delegate proxies (see PR castleproject#436) so that class can now be removed. The unit tests for it are rewritten in terms of the newer API to demonstrate that it is indeed a suitable replacement.
* Windsor needs `TypeUtil.GetAllInterfaces`, so make it public again. * Windsor is currently relying on `DelegateProxyGenerator`, but there is now another, more public API to build delegate proxies (see PR castleproject#436) so that class can now be removed. The unit tests for it are rewritten in terms of the newer API to demonstrate that it is indeed a suitable replacement.
This is a follow-up to #403, which started out with the goal of adding a set of
ProxyGenerator.CreateDelegateProxy
methods, and ended with us thinking about achieving delegate proxies through DynamicProxy's mixin capabilities. I am closing the earlier PR in favour of this present one.This PR adds the following capabilities to DynamicProxy:
void ProxyGenerationOptions.AddDelegateMixin(Delegate @delegate)
to add a delegate mixin. This will produce an
Invoke
method on the proxy that has the same signature as the delegate. The delegate can beProceed
-ed to.void ProxyGenerationOptions.AddDelegateTypeMixin(Type delegateType)
adds an
Invoke
method on the proxy that has the same signature as a specified delegate type. When thoseInvoke
methods are intercepted, they cannot be proceeded from as that mixin has no target.Delegate ProxyUtil.CreateDelegateToMixin(object target, Type delegateType)
TDelegate ProxyUtil.CreateDelegateToMixin<TDelegate>(object target)
are helper methods to create a delegate of a given type to a suitable
Invoke
method on a proxy.By combining the above methods, you can now create a delegate proxy:
Because you're creating a class (or interface) proxy, you have full control over the "container" type for the generated interceptable
Invoke
method. You're also free to add custom attributes to that type, as you normally would.I'd still like to...
run some tests whether these new capabilities are actually useful in consuming libraries (Moq in my case)See feedback for Moq and NSubstitute below. Feedback is better than anticipated.
I am also still unsure whether there may be serialization issues when using lambda delegates as mixin targets... and whether that is relevant in practice.This can be worked around by rewriting such lambdas & captures as explicit types. Beyond that, I'm not yet sure this is even fixable; if so, then we'd likely have to let
ProxyGenerationOptions
implementISerializable
and serialize it differently (specifically themixins
field).For now, I'd like to treat this as a known current limitation of the delegate mixin story, as it's probably not an immediate roadblock for most downstream libs.
Closes #399 (where we said that noone except DP itself should emit into its dynamic assembly, and the only known roadblock to that is delegate proxying; so if we get that, we no longer need
ModuleScope.DefineType
)./cc @blairconrad, @thomaslevesque, @zvirja