ObjenesisCglibAopProxy's fallback mode triggers duplicate class definition error [SPR-13131]

[spring-projects-issues]

Tobias Gesellchen opened SPR-13131 and commented

With #15223 CGLIB has been replaced by Objenesis without the ability to configure CGLIB as proxy factory.

We would like to use CGLIB proxying in a GAE environment, where Objenesis isn't allowed. We didn't find the org.springframework.aop.framework.DefaultAopProxyFactory to be configurable for our needs, where the ObjenesisCglibAopProxy instantiation is hard coded.

Would it be possible to allow the org.springframework.aop.framework.CglibAopProxy to be configured as default proxy factory?

---

Affects: 4.1.6

Issue Links:

• java.lang.VerifyError: $$EnhancerBySpringCGLIB$$744256ce, method: <init> signature: ()V) Illegal type in constant pool [SPR-13098] #17689 java.lang.VerifyError: $$EnhancerBySpringCGLIB$$744256ce, method: signature: ()V) Illegal type in constant pool
• Add ability to create proxy around classes that has no default constructor [SPR-10594] #15223 Add ability to create proxy around classes that has no default constructor
• ProxyFactory should not discard objenesis cache, thereby enabling efficient proxy instantiation [SPR-12755] #17352 ProxyFactory should not discard objenesis cache, thereby enabling efficient proxy instantiation
• Automated smoke test for GAE compatibility [SPR-13830] #18403 Automated smoke test for GAE compatibility

[spring-projects-issues]

Juergen Hoeller commented

More specifically, it's about using CGLIB without Objenesis... since ObjenesisCglibAopProxy still creates a CGLIB proxy class, just instantiating it via Objenesis in order to avoid constructor side effects.

That said, we do have a fallback path for regular CGLIB proxy instantiation if Objenesis fails for a given class. Does that not kick in on GAE for some reason? Or would you like to avoid the attempt to use Objenesis for efficiency reasons?

Juergen

[spring-projects-issues]

Tobias Gesellchen commented

You're right, it's about using CGLIB without Objenesis.

We had issues even with the fallback, but I cannot remember the actual error. I'll investigate tomorrow and add more details here.

[spring-projects-issues]

Tobias Gesellchen commented

It looks like GAE IsolatedAppClassLoader doesn't support repeated class creation, we get the following error:

9:36:22.027 WARNING com.google.apphosting.utils.jetty.JettyLogger warn : Nested in org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'ourClass': Initialization of bean failed; nested exception is org.springframework.aop.framework.AopConfigException: Could not generate CGLIB subclass of class [class com.example.OurClass]: Common causes of this problem include using a final class or a non-visible class; nested exception is org.springframework.cglib.core.CodeGenerationException: java.lang.reflect.InvocationTargetException-->null:
java.lang.LinkageError: loader (instance of  com/google/appengine/tools/development/IsolatedAppClassLoader): attempted  duplicate class definition for name: "com/example/OurClass$$EnhancerBySpringCGLIB$$fca8322"
	at java.lang.ClassLoader.defineClass1(Native Method)
	at java.lang.ClassLoader.defineClass(ClassLoader.java:800)
	at sun.reflect.GeneratedMethodAccessor21.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:606)
	at com.google.appengine.tools.development.agent.runtime.Runtime.invoke(Runtime.java:130)
	at org.springframework.cglib.core.ReflectUtils.defineClass(ReflectUtils.java:384)
	at org.springframework.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:219)
	at org.springframework.cglib.proxy.Enhancer.createHelper(Enhancer.java:377)
	at org.springframework.cglib.proxy.Enhancer.create(Enhancer.java:285)
	at org.springframework.aop.framework.CglibAopProxy.createProxyClassAndInstance(CglibAopProxy.java:227)
	at org.springframework.aop.framework.ObjenesisCglibAopProxy.createProxyClassAndInstance(ObjenesisCglibAopProxy.java:66)

[spring-projects-issues]

Tobias Gesellchen commented

Another aspect to make the Objenesis vs. plain CGLIB implementation configurable would be performance. Since we know that Objenesis will always fail on GAE, we would prefer to disable it completely and set plain CGLIB as default.

[spring-projects-issues]

Juergen Hoeller commented

This seems to be caused by CGLIB's Enhancer.createClass() which we're using in the Objenesis case instead of Enhancer.create() which creates an actual proxy instance right away. There may be a bug hiding in our specific use of CGLIB there, with it bypassing its own class cache; I'll have a deeper look for 4.2 RC2.

Objenesis itself even caches its instantiation strategy, so pretty much auto-adapts to an environment where it can't do its regular job, assumably even without a significant performance penalty. In any case, I'd prefer for this to be auto-adaptive, with no need to explicitly configure the strategy on Google App Engine.

Juergen

[spring-projects-issues]

Tobias Gesellchen commented

Great to hear that you'll have a deeper look for 4.2! In case you need us to test the RC2 in our environment, just leave a note here.

We would still prefer to have an option to explicitly select the CglibAopProxy, probably using a flag in DefaultAopProxyFactory or by providing an own AopProxyFactory, so that the Objenesis classes wouldn't even be loaded. Yet, the bugfix would be more important than any premature performance optimization ;-)

[spring-projects-issues]

Juergen Hoeller commented

More specifically, an Enhancer.create() call after Enhancer.createClass() and a subsequent Objenesis failure seems to be the problem here. Objenesis goes into its fallback mode and then things fail when we enter the regular CGLIB code path, probably because of the preceding createClass() call where the class hasn't been used at all.

There is clearly something at odds here. I'll deal with it from two perspectives: For a start, the fallback needs to be defensive enough to reliably work, possibly reusing the createClass() result even in the fallback code path; and we need to be efficient in taking the fallback route when we know that Objenesis is always going to fail.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Objenesis itself basically checks whether sun.misc.Unsafe is available, depending on the JVM. It doesn't really do anything out of the ordinary, so loading its facade shouldn't cause any harm. Let's deal with the CGLIB duplicate class problem and see how the end result of that effort performs.

That said, allowing for an explicit decision here is fine as well. It just shouldn't be necessary in any common environment, that's what I'm aiming for.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Tobias, could you please turn on debug logging for "org.springframework.aop.framework" to see the stacktrace for the ObjenesisException which triggers the fallback code path in your case? It'd be interesting to see how exactly it fails on Google App Engine. I suppose somewhere along the lines of easymock/objenesis#15 but it'd be good to have that verified.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

In the meantime, I got the fallback code path working through reuse of the createClass result. This part will also be backported to 4.1.7.

It's now just about optimizing the detection of the need for a fallback in the first place. Spring 4.2 has some revised caching of Objenesis instantiators already but it'd be good to pre-detect the "fails always" case as well, not bothering with an Objenesis instantiation attempt and an individual fallback every time in such a scenario.

[spring-projects-issues]

Tobias Gesellchen commented

the logs show this stacktrace:

15:11:31.717 [main] DEBUG o.s.a.f.ObjenesisCglibAopProxy - Unable to instantiate proxy using Objenesis, falling back to regular proxy construction
org.springframework.objenesis.ObjenesisException: java.lang.IllegalAccessException: Reflection is not allowed on public static sun.reflect.ReflectionFactory sun.reflect.ReflectionFactory.getReflectionFactory()
	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryHelper.createReflectionFactory(SunReflectionFactoryHelper.java:78) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryHelper.newConstructorForSerialization(SunReflectionFactoryHelper.java:39) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryInstantiator.<init>(SunReflectionFactoryInstantiator.java:38) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.objenesis.strategy.StdInstantiatorStrategy.newInstantiatorOf(StdInstantiatorStrategy.java:58) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.objenesis.ObjenesisBase.getInstantiatorOf(ObjenesisBase.java:91) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.objenesis.ObjenesisBase.newInstance(ObjenesisBase.java:73) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.aop.framework.ObjenesisCglibAopProxy.createProxyClassAndInstance(ObjenesisCglibAopProxy.java:57) ~[spring-aop-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.aop.framework.CglibAopProxy.getProxy(CglibAopProxy.java:202) ~[spring-aop-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.aop.framework.ProxyFactory.getProxy(ProxyFactory.java:109) ~[spring-aop-4.1.6.RELEASE.jar:4.1.6.RELEASE]

...

Caused by: java.lang.IllegalAccessException: Reflection is not allowed on public static sun.reflect.ReflectionFactory sun.reflect.ReflectionFactory.getReflectionFactory()
	at com.google.appengine.tools.development.agent.runtime.Runtime.verifyWhiteListed(Runtime.java:92) ~[appengine-agentruntime.jar:na]
	at com.google.appengine.tools.development.agent.runtime.Runtime.invoke(Runtime.java:127) ~[appengine-agentruntime.jar:na]
	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryHelper.createReflectionFactory(SunReflectionFactoryHelper.java:72) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]

with the fallback producing this stacktrace:

Caused by: org.springframework.cglib.core.CodeGenerationException: java.lang.reflect.InvocationTargetException-->null
	at org.springframework.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:237) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.cglib.proxy.Enhancer.createHelper(Enhancer.java:377) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.cglib.proxy.Enhancer.create(Enhancer.java:285) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.aop.framework.CglibAopProxy.createProxyClassAndInstance(CglibAopProxy.java:227) ~[spring-aop-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.aop.framework.ObjenesisCglibAopProxy.createProxyClassAndInstance(ObjenesisCglibAopProxy.java:66) ~[spring-aop-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.aop.framework.CglibAopProxy.getProxy(CglibAopProxy.java:202) ~[spring-aop-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	... 44 common frames omitted
Caused by: java.lang.reflect.InvocationTargetException: null
	at sun.reflect.GeneratedMethodAccessor21.invoke(Unknown Source) ~[na:na]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.7.0_79]
	at java.lang.reflect.Method.invoke(Method.java:606) ~[na:1.7.0_79]
	at com.google.appengine.tools.development.agent.runtime.Runtime.invoke(Runtime.java:130) ~[appengine-agentruntime.jar:na]
	at org.springframework.cglib.core.ReflectUtils.defineClass(ReflectUtils.java:384) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]
	at org.springframework.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:219) ~[spring-core-4.1.6.RELEASE.jar:4.1.6.RELEASE]

[spring-projects-issues]

Tobias Gesellchen commented

as a side note: I'm testing locally with the appengine sdk: -javaagent:/path/to/appengine-sdk/appengine-java-sdk-1.9.18/lib/agent/appengine-agent.jar

[spring-projects-issues]

Tobias Gesellchen commented

would you like us to check if the code path is different when run on the actual GAE and not locally?

[spring-projects-issues]

Juergen Hoeller commented

Yes please, that would be great to know!

The CodeGenerationException part is clear in the meantime, caused by the double CGLIB create invocation in that code path. This is fixed and will be backported to 4.1.7 as well.

So the remaining interesting part is the IllegalAccessException. If that can be reliably expected on production GAE as well, we can back out and bypass Objenesis from then onwards. That's an optimization I'm about to roll into 4.2 RC2, minimizing the Objenesis attempts to just one, immediately falling back to regular instantiation from then on. This is in line with other parts of the codebase where we're also backing out if some target API permanently fails. That should hopefully be fine for your purposes as well.

Juergen

[spring-projects-issues]

Tobias Gesellchen commented

looks like the expected IllegalAccessException doesn't appear on production. This is our stacktrace:

      Caused by: java.lang.NoClassDefFoundError: sun.reflect.ReflectionFactory is a restricted class. Please see the Google App Engine developer's guide for more details.
      	at com.google.appengine.runtime.Request.process-8527557b0ff645c5(Request.java) ~[na:na]
      	at sun.reflect.ReflectionFactory.<clinit>(ReflectionFactory.java) ~[na:na]
      	at java.lang.reflect.Method.invoke(Method.java:45) ~[na:1.7.0-google-v6]
      	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryHelper.createReflectionFactory(SunReflectionFactoryHelper.java:72) ~[spring-core-4.1.6.RELEASE.jar:na]
      	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryHelper.newConstructorForSerialization(SunReflectionFactoryHelper.java:39) ~[spring-core-4.1.6.RELEASE.jar:na]
      	at org.springframework.objenesis.instantiator.sun.SunReflectionFactoryInstantiator.<init>(SunReflectionFactoryInstantiator.java:38) ~[spring-core-4.1.6.RELEASE.jar:na]
      	at org.springframework.objenesis.strategy.StdInstantiatorStrategy.newInstantiatorOf(StdInstantiatorStrategy.java:58) ~[spring-core-4.1.6.RELEASE.jar:na]
      	at org.springframework.objenesis.ObjenesisBase.getInstantiatorOf(ObjenesisBase.java:91) ~[spring-core-4.1.6.RELEASE.jar:na]
      	at org.springframework.objenesis.ObjenesisBase.newInstance(ObjenesisBase.java:73) ~[spring-core-4.1.6.RELEASE.jar:na]
      	at org.springframework.aop.framework.ObjenesisCglibAopProxy.createProxyClassAndInstance(ObjenesisCglibAopProxy.java:57) ~[spring-aop-4.1.6.RELEASE.jar:na]
      	at org.springframework.aop.framework.CglibAopProxy.getProxy(CglibAopProxy.java:202) ~[spring-aop-4.1.6.RELEASE.jar:na]

[spring-projects-issues]

Juergen Hoeller commented

Thanks for checking! So that seems to be the same exception as reported in easymock/objenesis#15 - alright, we'll do a broader check then, covering a directly propagated NoClassDefFoundError (not even wrapped in an ObjenesisException apparently, therefore not even triggering the fallback code path yet) as well.

[spring-projects-issues]

Tobias Gesellchen commented

Ah, you're right, I didn't notice that the fallback code hasn't been triggered on production. So you would also backport to handle the NoClassDefFoundError to 4.1.7?

[spring-projects-issues]

Juergen Hoeller commented

Yes, making sure the fallback code path reliably kicks in is a 4.1.7 topic, and handling this NoClassDefFoundError is quite GAE-specific but nevertheless worth adding there.

For 4.2, we'll additionally track general Objenesis failure and will efficiently bypass it afterwards. I'm also considering a "spring.objenesis.ignore" system property with a boolean value, along the lines of "spring.beaninfo.ignore" which we have in CachedIntrospectionResults.

[spring-projects-issues]

Tobias Gesellchen commented

great, so I'm excited to check the upcoming releases. If you need more feedback or testing, please ask. Thanks for now!

[spring-projects-issues]

Juergen Hoeller commented

We'll have 4.2 and 4.1.7 snapshots ready for testing tomorrow morning. 4.2 RC2 and 4.1.7 are scheduled for release on June 19 (i.e. this Friday), so your report came just in time :-)

Juergen

[spring-projects-issues]

Tobias Gesellchen commented

yay! :-)

[spring-projects-issues]

Juergen Hoeller commented

The 4.2 story is complete now as per the description above, not just for CGLIB AOP proxies but also for CGLIB-created FactoryBean proxies in configuration classes and for controller proxies in the MVC URI component builder. All of the smart Objenesis handling has been baked into 4.2's new SpringObjenesis class (introduced back in #17352 for proper instantiator caching).

I'll let you know once the 4.1.7 backport is available. That part will be a local fix in ObjenesisCglibAopProxy only.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Tobias Gesellchen, this is available in the latest 4.2.0.BUILD-SNAPSHOT and 4.1.7.BUILD-SNAPSHOT now. It would be great if you could give it a try before Friday!

Juergen

[spring-projects-issues]

Tobias Gesellchen commented

Juergen, I gave 4.1.7.BUILD-SNAPSHOT a try, with success on my local environment. I'm afraid it won't work on production, because the fallback won't be triggered on a java.lang.NoClassDefFoundError. Did I miss something?

[spring-projects-issues]

Juergen Hoeller commented

Oops, indeed, the NoClassDefFoundError part is currently hiding in the 4.2-only SpringObjenesis delegate, transparently turned into an ObjenesisException there. I need to merge that into 4.1.7's ObjenesisCglibAopProxy code still...

Juergen

[spring-projects-issues]

Juergen Hoeller commented

The next 4.1.7 snapshot (ready in ~15 min) should have this properly addressed! Thanks for the immediate testing...

[spring-projects-issues]

Tobias Gesellchen commented

It works on production now (with 4.1.7 snapshot), thanks for the quick fix!