Forward non-intercepted methods on class proxies to target #571
Conversation
stakx
force-pushed
the
forward-non-intercepted-methods-to-target
branch
from
ea85307
to
547cd8f
Compare
| /// 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.
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.
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
stakx
force-pushed
the
forward-non-intercepted-methods-to-target
branch
from
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.
stakx
force-pushed
the
forward-non-intercepted-methods-to-target
branch
from
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.