ProxyGenerator cache does not take into account AdditionalAttributes about core HOT 19 CLOSED

Yup. This is a known... Unsupported scenario.

Why so you need this?

On Wed, 14 Jan 2015 09:13 Christian Merat [email protected] wrote:

When generating Proxy types, the CacheKey does not take into account the
AdditionalAttributes specified on the ProxyGenerationOptions which causes a
cache hit to return a Proxy that does NOT have the AdditionalAttributes
applied to it if a previous Proxy class has already been generated for
the same type.

The solution to fix this is for the Equality/GetHashCode operation on
ProxyGenerationOptions to take into consideration the AdditionalAttributes
and cause a cache miss, forcing the generation of a new type that will have
the correct attributes applied to it.

—
Reply to this email directly or view it on GitHub
#77.

from core.

I'm using a mocking framework (https://github.com/FakeItEasy/FakeItEasy) that uses ProxyGenerator to generate its fake instances. I need to test logic based on Attributes that are applied to a type. My unit tests are failing because the first proxy type definition is sticking to all other test cases (since the AppDomain is not unloaded between unit tests). The fake class with custom attributes was the simplest way to test the algorithm.

from core.

Can you post some of that code?

On Wed, 14 Jan 2015 09:22 Christian Merat [email protected] wrote:

I'm using a mocking framework (https://github.com/FakeItEasy/FakeItEasy)
that uses ProxyGenerator to generate its fake instances. I need to test
logic based on Attributes that are applied to a type. My unit tests are
failing because the first proxy type definition is sticking to all other
test cases (since the AppDomain is not unloaded between unit tests). The
fake class with custom attributes was the simplest way to test the
algorithm.

—
Reply to this email directly or view it on GitHub
#77 (comment).

from core.

Even better, I can link to the issue that I've opened on their project: FakeItEasy/FakeItEasy#436

It contains repro steps as well as my investigation into what is causing the issue.

from core.

right.

@cmerat a workaround for time being would be to disable caching (override ModuleScope.GetFromCache to return null.

In the longer term, perhaps this would be an acceptable workaround for FakeItEasy too. I see this very much as a corner case and I'm not really convinced fixing it, and therefore taking a performance hit for all other use-cases is a worthwhile trade of.

from core.

I guess it's worth noting this was also mentioned in #71

from core.

Would adding AdditionalProperties to the CacheKey really be that much of a performance hit? Edge case or not, the interface is essentially lying (GenerateProxy with the specified options will SOMETIMES not return a proxy class matching the options). In the end it's your project and I understand the need to decide where resources are allocated. If you're open to contributions I'm willing to adjust GetHashCode and Equals to take into consideration AdditionalProperties.

from core.

Go for it

On Wed, 14 Jan 2015 11:46 Christian Merat [email protected] wrote:

Would adding AdditionalProperties to the CacheKey really be that much of a
performance hit? Edge case or not, the interface is essentially lying
(GenerateProxy with the specified options will SOMETIMES not return a proxy
class matching the options). In the end it's your project and I understand
the need to decide where resources are allocated. If you're open to
contributions I'm willing to adjust GetHashCode and Equals to take into
consideration AdditionalProperties.

—
Reply to this email directly or view it on GitHub
#77 (comment).

from core.

Great - we'll hold off on the FakeItEasy workaround then. From our POV it would definitely be better for this to be fixed in Castle.

from core.

@kkozmic The pull request #78 is up which contains the necessary corrections.

from core.

#78 is now merged, it'll be in the next release.

from core.

Thanks, @cmerat, @kkozmic, and @jonorossi!

from core.

I just looked at the fix for this in #78, and I don't think it actually works. As noted by @blairconrad in FakeItEasy/FakeItEasy#437 (comment), it seems to work, but only because in the unit tests, the ProxyGenerationOptions are created using the same instances of CustomAttributeBuilder, not distinct identical instances. CustomAttributeBuilder doesn't override Equals, so instances are compared by reference. So the tests pass, but only "by accident"... if you change the tests to use distinct but identical instances of CustomAttributeBuilder, several of them fail.

Worse: CustomAttributeBuilder doesn't expose its members publicly, so there's no way to write a custom equality comparer for it...

from core.

I think there's a way to fix it, but it would involve a change in the Castle.Core API (not necessarily a big issue since it's a major release anyway): instead of using CustomAttributeBuilder directly to specify additional attributes, use a custom type that implements equality properly and can be converted to a CustomAttributeBuilder.

from core.

Thanks @thomaslevesque, I'm happy to entertain the idea of a Castle specific immutable type to add custom attributes and emulate CustomAttributeBuilder. Were you thinking it would have 4 constructors to match CustomAttributeBuilder and would new up a CustomAttributeBuilder in its constructor so that any bad set up would throw from the CustomAttributeBuilder code?

It is probably worthwhile keeping AdditionalAttributes but marking it obsolete like the old AttributesToAddToGeneratedTypes property (looking at GitHub people still use the old one), and implementing the caching to declare anything with an AdditionalAttributes specified as not-equal, or probably better yet just ignore the property as DP v3 did with a comment in the code stating this is the desired behaviour not a bug.

Do you have time to spike something for review?

from core.

@jonorossi, thanks for the speedy response!

Probably you'd want to ignore the AdditionalAttributes property as v3 does, or run the risk of bloating the type cache. Besides, if people have been content with the current behaviour, it won't bother them. I doubt anyone relies on the different sets of additional attributes not being applied, but just in case, keeping the old behaviour lessens the risk of alienating people.

And if I know @thomaslevesque, he's already working on the spike!
And if it turns out that he hasn't time, I can take a stab.

from core.

Probably you'd want to ignore the AdditionalAttributes property as v3 does, or run the risk of bloating the type cache. Besides, if people have been content with the current behaviour, it won't bother them. I doubt anyone relies on the different sets of additional attributes not being applied, but just in case, keeping the old behaviour lessens the risk of alienating people.

Thanks @blairconrad, that's exactly the thought process I had.

from core.

Were you thinking it would have 4 constructors to match CustomAttributeBuilder and would new up a CustomAttributeBuilder in its constructor so that any bad set up would throw from the CustomAttributeBuilder code?

Yes, unless you have a better idea in mind. The CustomAttributeBuilder API isn't terribly convenient IMO, so maybe I'll try something different.

It is probably worthwhile keeping AdditionalAttributes but marking it obsolete like the old AttributesToAddToGeneratedTypes property (looking at GitHub people still use the old one)

I guess it would make things easier for users of older versions, but that means the name AdditionalAttributes isn't available and we have to come up with something else. Ideally, I'd rather replace the current property completely. It would be a breaking change, but it wouldn't require lots of effort to adapt existing code.

Do you have time to spike something for review?

I'll submit a PR as soon as I can (probably tonight or tomorrow)

from core.

Great idea @thomaslevesque.

During my initial implementation, I did notice that CustomAttributeBuilder only has a default Equals implementation which would cause distinct instances with the same values to not match as equal even though they are functionally equivalent. The initial objective however was to simply take the attribute into account for the type cache. You are right that the side-effect of this is that for the equivalent Attribute, there would be two distinct types in the type cache. The only time where you'd get a cache hit is when it's the exact same instance. At the time, I did not feel this was a blocking scenario since I would get the benefits of different types at the cost of additional cache.

Thanks for tackling a more robust solution to this issue. I agree that your solution will yield superior results in all cases.

from core.