>> I remove:
>> - the requirement of a static or instance field (+ initialization)
> re: I think static would be better, then you pay the price of initialization
> only once
True, although the price of initialization is very small with a
SelectingInterceptor.
>> - the requirement of having IInterceptorSelector in the
>> ProxyGenerationOptions (+ serialization, + Equals, + GetHashCode)
> re: but I think PGO is where it belongs.
Is that so? If it works at runtime (rather than _proxy generation
time_), why does it belong to the ProxyGenerationOptions? One could
argue that the items in the ProxyGenerationOptions should specify
options for the proxy generation. (That we have the mixins in there is
bad enough.)
> Also if we codegen it, it would work slightly faster at runtime (no need to
> lookup the methodInfo, then lookup interceptors for the method)
Slightly faster, that's true. But only very slightly. The MethodInfo
is looked up anyway (usually from the cache), the interceptor lookup
would be via field instead of via Dictionary. The price is one
additional instance field per method, but I'm not sure whether we are
interested in proxy size at all.
> Also code for serialization and Equals and GHC is already there :)
Yes, but it needs to be maintained! Serialization for
IInterceptorSelector has long been missing without anybody noticing.
DynamicProxy is tested mostly via integration tests, not unit tests,
so omissions are not so unlikely to stay unnoticed for a long time.
Also, your patch adds a substantial amount of code, which also needs
to be maintained. The DP code is already very complicated, that's why
I avoid extending it whenever possible.
And the code generation stuff needs to be tested on different
configurations with different versions .NET (2.0, 2.0 SP1, 2.0 SP2)
due to all those Reflection.Emit and generics-related runtime bugs in
those versions.
> Another thing is, that it'd be awkward: you'd have an array of interceptors
> in the proxy type, but keep only one interceptor in it, and all the call next
> logic would be duplicated in it.
Yes, that's true, I omitted that in my sample (I didn't have much time
to spend on the issue yesterday). We could mitigate this, however, as
follows:
public class SelectingInterceptor : IInterceptor
{
private IInterceptor[] nestedInterceptors;
private IInterceptorSelector selector;
public SelectingInterceptor (IInterceptor[] nestedInterceptors,
IInterceptorSelector selector)
{
...
}
public void Intercept (IInvocation invocation)
{
var selectedInterceptors = selector.SelectInterceptors
(invocation.TargetType, invocation.Method);
invocation.ResetInterceptors (selectedInterceptors); //
alternatively create a new invocation object
invocation.Proceed();
}
}
Ie. leave the logic in AbstractInvocation, but have the selection in
an interceptor.
> It also adds an implicit contract: put any interceptors you like into the
> CreateClassProxy, or if you want to select some of them, wrap them with
> SelectingInterceptor. This is the kind of implicit contracts I hate Unity for.
> We might add an overload that takes just a single SelectingInterceptor but I
> don't like the idea of adding another overload to every CreateXYZProxy method.
I don't see this as an implicit contract. It's just a feature
implemented as a simple composite pattern, I think it would be quite
easy to grasp.
> From current implementation' perspective, your idea certainly introduces less
> changes (virtually none, if we ditch the add-overloads idea), but in a longer
> perspective, I think Selector on ProxyGenerationOptions would work better.
> Also, sole implementation of SelectingInterceptor would be more complicated
> than adding support for IInterceptorSelector via code gen.
But in a different place, where you can unit-test (instead of
integration-test), and which is far easier to read, understand, and
maintain.
You know, I don't prefer the idea of SelectingInterceptor on
principle, I prefer it _only_ on terms of maintainability and
testability. And the reason for this is that I have made _many_
experiences with faulty code generation with DP in the past. For
example, the patch you submitted has a problem which only occurs under
some circumstances on some versions of the .NET framework (see below).
This is a nightmare, and it's the reason why I prefer to pull out as
much from Reflection.Emit as possible.
Hammett asked for the use cases, but I don't think that question is
valid: The two approaches have identical functionality and slightly
different performance (the code-gen way is slightly faster, the
SelectingInterceptor stuff generates smaller classes and moves the
cache to a dictionary).
I want to add that I don't have an immediate use case for the feature.
When DP 2 was created, I thought I would have one with re-motion
Mixins; but it turned out that I had to generate my own proxies
anyway. (I'm still using DP2's AST stuff, though.)
> what do others think? is there a third way?
I just thought of a third way: Pushing the selection stuff into
AbstractInvocation... That would address most of the points you make
(especially that of interface: to the user, it would feel exactly like
your implementation), while also addressing my point of not adding
codegen when avoidable :)
I think, lastly Jonathon has to make that decision as the project
lead. I'm Cc'ing him in case the thread hasn't caught his eye yet.
And, regarding your patch, I've finally reviewed it, here are my comments:
- Please use tabs instead of spaces :)
BaseProxyGenerator.cs:
- ImplementProxiedMethod:
] MethodInfo genericMethod = method.MakeGenericMethod(genericMethodArgs);
] tokenFieldName = genericMethod.Name;
] methodInfoTokenExp = new MethodTokenExpression(genericMethod);
The tokenFieldName is not guaranteed to be unique considering
overloaded methods.
The MethodInfo generated by methodInfoTokenExp is not guaranteed to be
a closed generic method previous to .NET 3.5 (2.0 SP1); see
AbstractInvocation.GetConcreteGenericMethod.
- HandleNoInterceptorsSelectedCase:
Suggestion: Maybe it would be better to invoke
IInvocation.InvokeMethodOnTarget to have this logic in one place. Or
at least, you could try to consolidate the code for IInvocation and
HandleNoInterceptorsSelectedCase instead of duplicating it.
Suggestion: With the current implementation, maybe better make the
method abstract and override it in all concrete implementations? Two
of them seem to override the implementation anyway.
- HandlePossibleLeakingThisCall:
I think it's problematic if we automate this for the no-interceptors
case, I would take it out of this patch and discuss it separately.
(Especially after the comment Hammett made above in this thread.)
ClassProxyGenerator.cs:
- ] //todo: to robic tylko jak jest abstrakcyjna, w przeciwnym wypadku
wolac bazowa implementacje
I don't speak Polish, but it seems something related to abstract
things and implementations is missing? :)
- MethodInvocationExpressionEx.cs:
I suggest finding a better name for this. Although I see why you did
this. I wonder whether it is possible to simply change
MethodInvocationExpression to take an Expression as the target instead
of a Reference? Converting References to Expressions is easy,
converting Expressions to References is not.
Best regards,
Fabian
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Castle Project Development List" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/castle-project-devel?hl=en
-~----------~----~----~----~------~----~------~--~---
