Prevent member name collision when proxy implements same generic interface more than twice by stakx · Pull Request #285 · castleproject/Core

stakx · 2017-07-02T21:50:56Z

After #89, this is a second attempt at fixing #88.

When a proxy is made to implement the same generic interface more than twice, and that generic interface contains a method that does not use any generic type parameters, this can result in a member name collision. This would happen e.g. with a proxy implementing IObserver<T> three times.

DynamicProxy already checks for member name collisions, and when it detects one, it switches to explicit interface implementation and prefixes member names with the interface's type name. For the above example, this will add the following methods to the proxy:

OnCompleted
IObserver`1.OnCompleted
IObserver`1.OnCompleted

Because the actual type arguments are omitted and only their count is included, the current collision checks are not sufficient. This commit makes explicitly implemented methods' names unique by including the full type names of the generic type arguments; for instance:

OnCompleted
System.IObserver`1[[System.Int32, mscorlib, Version=…, Culture=…, PublicKeyToken=…]].OnCompleted
System.IObserver`1[[System.String, mscorlib, Version=…, Culture=…, PublicKeyToken=…]].OnCompleted

(I've omitted some detail above for brevity, using ….)

This is about the simplest possible solution to #88, but I'm not sure if you are OK with these kinds of member names. They are unique, but also long and ugly (which could be partly mitigated by using System.CodeDom to produce more C#-like type names), and they contain whitespace and other special characters; I have no idea whether these are always safe for proxy consumers.

Feel free to request modifications or to close this PR if it's not acceptable.

stakx · 2017-07-04T09:04:55Z

src/Castle.Core.Tests/BasicInterfaceProxyTestCase.cs

 			Assert.IsNotNull(method);
 			Assert.AreSame(method, type.GetTypeInfo().GetRuntimeInterfaceMap(typeof(IIdenticalOne)).TargetMethods[0]);
-			MethodInfo method2 = type.GetMethod("IIdenticalTwo.Foo", BindingFlags.Instance | BindingFlags.Public);
+			MethodInfo method2 = type.GetMethod($"{typeof(IIdenticalTwo).FullName}.Foo", BindingFlags.Instance | BindingFlags.Public);


The reason why I chose not to hardcode the full type name here is because it includes things like DLL version numbers and public keys, which I wasn't sure whether they are the same on Mono and the CLR, and they're irrelevant to the test anyway.

jonorossi · 2017-07-04T11:14:52Z

As you suspected, I'm not a fan of those member names. It appears they also won't be CLS compliant with the spaces: https://docs.microsoft.com/en-us/dotnet/standard/language-independence-and-language-independent-components#naming, I'm also not sure dots would be compliant but they don't seem to be mentioned. However, I'm not sure CLS compiliance really matters since no one is going to manually call them from language compiled code. I'd hate to see a stacktrace with them though, would be awfully confusing.

What do the CodeDom identifiers look like?

I'd be happy with using just the parameter short names (i.e. not namespace or assembly-qualified) which is what is already done for the first bit (e.g. IIdenticalTwo rather than Castle.DynamicProxy.Tests.Interfaces.IIdenticalTwo):

OnCompleted
IObserver`1.OnCompleted
IObserver`1.OnCompleted

OnCompleted
IObserver`1[Int32].OnCompleted  // <<<
IObserver`1[String].OnCompleted // <<<

The docs for Type.GetType state that the extra set of brackets is only needed around assembly-qualified types, so the format would look like this for multiple parameters:

IObserver`2[Int32,String].OnCompleted

Unless I missed it I can't see any property in the BCL on a type to get the unqualified name as well as its generic parameters, but should be easy to implement.

stakx · 2017-07-04T11:37:16Z

What do the CodeDom identifiers look like?

I only mentioned this as a potential route to take, I haven't actually tried it out just yet. The general idea is that CodeDom can be used to create type names that look more like C# rather than IL (e.g., <> brackets for generic type arguments instead of the backtick and the [] brackets). My understanding of this technique is that it is customizable to some extent (e.g. whether you want to include namespaces or not). That all being said, it might actually be better to keep an additional subsystem like CodeDom out of the picture, if it's not absolutely required.

IObserver`2[Int32,String].OnCompleted

I like that suggestion. The resulting member names won't be guaranteed to be unique (e.g. if you use two types NamespaceA.Foo and NamespaceB.Foo as type arguments), but in practice, that probably won't matter at all in most cases.

Shall I go ahead and change the PR accordingly?

jonorossi · 2017-07-04T11:45:06Z

That all being said, it might actually be better to keep an additional subsystem like CodeDom out of the picture, if it's not absolutely required.

Completely in agreement, not even sure if .NET Core has CodeDom.

The resulting member names won't be guaranteed to be unique

Agreed, it'll be exactly like the base type right now, you'd be silly to inherit/implement two Foo classes/interfaces 😉

Shall I go ahead and change the PR accordingly?

Yep, don't forget the changelog. Thanks.

When a proxy is made to implement the same generic interface more than twice, and that generic interface contains a method that does not use any generic type parameters, this can be result in a name collision. This would happen e.g. with a proxy implementing `IObserver<T>` three times. DynamicProxy does check for member name collisions, and when such a one is detected, it switches to explicit interface implementation and prefixes the member name with the generic interface type name. For the above example, this will add the following methods to the proxy: * OnCompleted * IObserver`1.OnCompleted * IObserver`1.OnCompleted It is clearly not sufficient to exclude the concrete type arguments and use only the number of type parameters. This commit changes the naming of explicitly implemented interface methods such that they are unique by including the generic type arguments. For example: * OnCompleted * IObserver`1[Boolean].OnCompleted * IObserver`1[Int32].OnCompleted

stakx · 2017-07-04T20:25:18Z

I think something might still be missing here: This PR only takes care of method name collisions, but does nothing about events and properties. I'll try to see if a similar change needs to be made to the other types overriding MetaTypeElement.SwitchToExplicitImplementation (MetaEvent.cs, MetaProperty.cs).

ghost · 2017-07-04T20:33:30Z

src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs

+			{
+				stringBuilder.Append('[');
+				var genericTypeArguments = type.GetGenericArguments();
+				for (int i = 0, n = genericTypeArguments.Length; i < n; ++i)


This method might introduce a performance hit on generation. It is recursive.

Was just basing that on a rudimentary find of usages for MetaMethod.

That's correct. The recursion depth will be proportional to the nesting depth of the (generic) type for which it generates a name. Non-generic types, however, will not trigger any recursion.

It fixes the original problem, but should we flag performance as a feature and make it part of the proxy options with a sensible default?

I don't think the performance will be affected. This only gets run when SwitchToExplicitImplementation is called once per MetaMethod and it is then cached in its name field. The StringBuilder will perform pretty similar to the existing string.Format and users are rarely going to have more than a single level of generic nesting, and even then the cost here is negligible compared to the actual type generation or other work DP does.

jonorossi · 2017-07-05T11:15:57Z

Looks good so far, just need to address properties and events.

stakx · 2017-07-05T21:51:57Z

@jonorossi - Done.

I've extracted the logic for creating explicit implementation member names into a separate helper class MetaTypeElementUtil, since it's identical for all types of members (methods, events, and properties).

This opens up an opportunity to minimize unnecessary heap allocations. @Fir3pho3nixx, like you showed in your review before, generating names for explicitly implemented members is possibly on a hot code path, so it should be reasonably efficient. It would be silly, for instance, having to instantiate 3 StringBuilder objects for just one property (1 for the property name, 1 for the getter method name, 1 for the setter method name). That's why I introduced a [ThreadStatic] StringBuilder that gets reused over and over again.

ghost · 2017-07-05T22:11:46Z

@stakx - In this context I think it plays well with async. There is a caution about the use of ThreadStatic. We also switched this out for use of AsyncLocal for netcore.

stakx · 2017-07-05T22:53:51Z

@Fir3pho3nixx - Thanks for checking up on my use of [ThreadStatic]. I was initially unsure whether it was really that easy to work with, but I've come to believe it should be perfectly fine in this particular case.

If my understanding of the TPL and async / await is correct, that Stack Overflow question revolves around the topic of task scheduling: is the continuation (i.e. the code after the await) guaranteed to execute on the same thread as the code before the await? (And the general answer to that is "no".)

I am almost certain that won't be an issue here, for the simple reason that our code in question is entirely synchronous. It should be impossible that code execution would switch from one thread to another all of a sudden and thus end up "stealing" the original thread's StringBuilder.

Unless I'm mistaken with any or all of the above, AsyncLocal<T> would be overkill here, because we have no Task or async / await induced async boundary across which we need to persist state.

ghost · 2017-07-06T06:15:03Z

@stakx - I agree. It is all sync so it should be fine.

jonorossi · 2017-07-06T15:42:45Z

In this context I think it plays well with async

Agreed. In general ThreadStatic will not work correctly in async/await, however there are no continuations in this code.

This opens up an opportunity to minimize unnecessary heap allocations.

What concerns me is that ThreadStatic and TLS are not free, the CLR has to call a helper function to access TLS to fetch that pointer for you, and this means the JIT can't aggressively optimise around accesses to this field. StringBuilders are fairly cheap and they'd only end up in Gen0 so would be efficiently garbage collected, and would mean StringBuilders aren't left around on the heap with pointers also left in TLS after generation is done (especially something long lived like a web app). I haven't done any performance tests, but I did come across one comment that said accessing a ThreadStatic field can be slower than an array allocation on .NET 3.5 (StringBuilder's constructor is pretty much just doing a char array allocation).

With that said the .NET Framework does actually have an internal StringBuilderCache class which is used by methods including String.Format, String.Join and String.Concat(IEnumerable<>). The class is a little more complex to optimise reuse of StringBuilders with enough capacity, because of fragmentation and internally StringBuilders are a linked list of StringBuilders to prevent any of it being allocated on the large object heap.

In this case it appears there is enough evidence that this will likely improve performance by some margin, but the performance improvement is currently unknown and likely negligible compared to the reflection calls to get the type names along with generic types; and this is all small compared to the reflection emit time.

It is best to avoid premature optimization (especially premature micro-optimisations) without measuring/benchmarking especially when the readability is hindered. For example, you could add a fast path to CreateNameForExplicitImplementation that avoids the StringBuilder if it isn't a generic type and call string.Concat(type.Name, ".", name) so it hits String's FastAllocateString method.

Let me know your thoughts, and if you disagree.

stakx · 2017-07-06T17:04:42Z

@jonorossi: Your response makes perfect sense to me.

I've heard similar things about the relatively large overhead of [ThreadStatic] on .NET 3.5. This appears to have been remedied in .NET 4, where the overhead of using [ThreadStatic] is said to be almost negligible. (It's also used in .NET 4's StringBuilderCache, after all.)

My overall impression is that using [ThreadStatic] would probably be slightly beneficial on .NET 4 and later, but hurt performance on .NET 3.5, so I'm adding a commit that removes this premature optimization. If ever you decide to drop support for .NET 3.5, you might want to revert that commit.

ghost · 2017-07-06T22:42:36Z

Thanks for making this thread interesting. Did not know threadstatic performance issue existed(or was fixed), just generally avoided it like herpes altogether :) Learn't ~~something~~ heaps 👍

jonorossi · 2017-07-07T09:39:19Z

@stakx thanks for making the change, and for getting this fix in. Merging.

stakx force-pushed the generic-interfaces-name-collisions branch from 91a5ea2 to 77cfec0 Compare July 4, 2017 09:02

stakx commented Jul 4, 2017

View reviewed changes

jonorossi added this to the vNext milestone Jul 4, 2017

stakx force-pushed the generic-interfaces-name-collisions branch from 77cfec0 to 55192ae Compare July 4, 2017 20:19

ghost reviewed Jul 4, 2017

View reviewed changes

stakx mentioned this pull request Jul 5, 2017

ProxyGenerationException using 3+ observer devlooped/moq#167

Closed

jonorossi approved these changes Jul 5, 2017

View reviewed changes

stakx force-pushed the generic-interfaces-name-collisions branch from 5047f76 to b3ea5a3 Compare July 5, 2017 21:41

stakx added 2 commits July 6, 2017 20:19

Avoid name collisions for properties & events, too

dd4f853

Remove [ThreadStatic] from MetaTypeElementUtil

d5c941f

stakx force-pushed the generic-interfaces-name-collisions branch from 01d4085 to d5c941f Compare July 6, 2017 18:19

jonorossi approved these changes Jul 7, 2017

View reviewed changes

jonorossi merged commit 5c947cb into castleproject:master Jul 7, 2017

jonorossi mentioned this pull request Jul 7, 2017

ProxyGenerationException proxying the same generic interface multiple times #88

Closed

stakx deleted the generic-interfaces-name-collisions branch July 7, 2017 10:27

stakx mentioned this pull request Aug 22, 2019

"Duplicate element: MetaMethod" for specific combinations of namespace & type names #469

Closed

Conversation

stakx commented Jul 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stakx Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonorossi commented Jul 4, 2017

Uh oh!

stakx commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonorossi commented Jul 4, 2017

Uh oh!

stakx commented Jul 4, 2017

Uh oh!

ghost Jul 4, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

stakx Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

jonorossi Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

jonorossi commented Jul 5, 2017

Uh oh!

stakx commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 5, 2017

Uh oh!

stakx commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 6, 2017

Uh oh!

jonorossi commented Jul 6, 2017

Uh oh!

stakx commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 6, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonorossi commented Jul 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stakx commented Jul 2, 2017 •

edited

Loading

stakx Jul 4, 2017 •

edited

Loading

stakx commented Jul 4, 2017 •

edited

Loading

ghost Jul 4, 2017 •

edited by ghost

Loading

stakx Jul 4, 2017 •

edited

Loading

stakx commented Jul 5, 2017 •

edited

Loading

stakx commented Jul 5, 2017 •

edited

Loading

stakx commented Jul 6, 2017 •

edited

Loading

ghost commented Jul 6, 2017 •

edited by ghost

Loading