-
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
Forward non-intercepted methods on class proxies to target #571
Forward non-intercepted methods on class proxies to target #571
Conversation
ea85307
to
547cd8f
Compare
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.
A few explanations on some of the trickier bits.
/// The <paramref name="hook"/> will get invoked for non-interceptable method notification only; | ||
/// it does not get asked whether or not to intercept the <paramref name="method"/>. | ||
/// </remarks> | ||
protected bool AcceptMethodPreScreen(MethodInfo method, bool onlyVirtuals, IProxyGenerationHook hook) |
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.
I am splitting off the pre-screening from AcceptMethod
so that WrappedMembersCollector
has a way of distinguishing between non-interceptable methods (e.g. object.Finalize
, object.MemberwiseClone
), and methods that the hook wants filtered out. It is only the latter group of methods that should be forwarded to the target; the former group of methods must not be proxied at all!
(In my original PR, I made this distinction via AllMethodsHook.SkippedTypes
, which wasn't ideal; see #450 (comment).)
{ | ||
var @delegate = GetDelegateType(method, proxy); | ||
var contributor = GetContributor(@delegate, method); | ||
var invocation = new CompositionInvocationTypeGenerator(targetType, method, null, false, contributor) | ||
.Generate(proxy, namingScope) | ||
.BuildType(); | ||
return new MethodWithInvocationGenerator(method, | ||
proxy.GetField("__interceptors"), | ||
skipInterceptors ? NullExpression.Instance : proxy.GetField("__interceptors").ToExpression(), |
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.
TBH this may be a bit of a hack: It relies on AbstractInvocation.Proceed
calling InvokeMethodOnTarget
right away when interceptors == null
(see here), so we get the needed behavior without having to implement a new type of MethodGenerator
. But the generated proxied method body in this case perhaps does too much work, and could be optimized further.
At this time, I don't yet understand the MethodGenerator
part of DP well enough to add a new variant. I may pick this up in the future, when the code base has been cleaned up a little more and when I'm more familiar with it. I'm hoping this will do in the meantime.
src/Castle.Core/DynamicProxy/Generators/MethodWithInvocationGenerator.cs
Outdated
Show resolved
Hide resolved
547cd8f
to
fe428b9
Compare
The (failing) tests being added here specify that if one chooses via `IProxyGenerationHook.ShouldInterceptMethod` not to intercept a method then invocations of that method on the proxy should be forwarded to the target object (if present).
Using `IndirectlyCalledMethodGenerator` for non-public methods even when `method.Proxyable == false` is wrong because it will cause interceptors to be invoked. But if the hook decided that a method shouldn't be inter- cepted, that is obviously wrong.
fe428b9
to
77743ac
Compare
This is a redo of my previous PR #450, which got stalled because I was under the impression that forwarding to non-public methods on the target would require special handling (and thus a lot of additional work). I've since discovered that the necessary bits are already in place:
When one proceeds from an intercepted non-public method of a class proxy to the target, the target method will likely be non-public, too, and therefore not directly callable from the outside. DynamicProxy solves this by calling the non-public target method through a delegate. This present PR simply piggybacks on that mechanism.
I found an issue with delegate type generation while working on this; see #570, which is a prerequisite for this PR and needs to be merged first.
(#570's commits are included here to avoid unrelated test failures; I'll rebase them away once it has been merged. For now, please ignore the first three commits of this PR.)Update: I've merged #570 and updated this PR.Fixes #67, fixes #536.