New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to create proxy around classes that has no default constructor [SPR-10594] #15223
Comments
Fedor Bobin commented If you not autorized to add artifacts to SpringSource maven repo mirror, you will get: To solve it you can uncomment line 70 in build.gradle (to enable fetching from maven central repo). |
Oliver Drotbohm commented I've submitted a slightly less changing pull request that also contains a few test cases at #327. I didn't include the issues around Currently the test cases all run against the newly created Objenesis proxies. We should probably tweak the build to run the tests for spring-context against plain CGLib as well (excluding Objenesis from the classpath). |
Fedor Bobin commented There is other problem with serialization: if class implements method readObject(java.io.ObjectInputStream in) and do some work or field checking in it some unexpectable effects can be performed. So we need to make proxies Externalizable to prevent such problems (In old implementation this problem was negligible because objects was properly constructed). |
Juergen Hoeller commented Oops, sorry for the "Waiting for Triage" there... We do mean to include this in 4.0, and Oliver has been pushing for it repeatedly. It just didn't make M3 since both Oliver and myself ran out of time. I'm properly scheduling it for 4.0 RC1 now. Juergen |
Juergen Hoeller commented A couple of notes:
Juergen |
Juergen Hoeller commented I guess if we're embedding it, the problem of Objenesis being a flaky dependency goes away. If necessary, we could check for the presence of a default constructor and only actually use Objenesis if we can't find one, staying with regular CGLIB construction otherwise. So I'm leaning towards embedding Objenesis 2.0 and always using it that way. Implementation-wise, that means we could roll Objenesis support into CglibAopProxy itself. Thoughts? Opinions? |
Fedor Bobin commented
No it does not. Also it can be strange for user if side-effects in constructor will take effect only if constructor has no arguments. I think if we use no-constructor-call object creation we should use same strategy for default constructor and constructor with arguments.
I do not know any disadvantages. If we will use repacked version we can use Objenesis in all cases. |
Phil Webb commented Although it is only 32K, I am not sure that we should inline Objenesis. I don't think we will see the same problems that we did with CGLib where various different libraries need different versions. I am also a little concerned that Objenesis may have unexpected side effects on some JREs since it appears to use If we do make Objenesis the default for CGLIB proxy creation I think we need an escape hatch to disable it if there are problems. With that in mind, I think that I prefer the idea of the optional dependency, at least for Spring 4.0. |
Fedor Bobin commented
Note: Objenesis is used by test frameworks (jmock-legacy, mockito etc...). List of supported VMs: So vm-specific classes for this VMs should be in classpath. |
Juergen Hoeller commented Fedor, good point about potential side effects when executing the default constructor. So if we use Objenesis by default, we should use it for all CGLIB proxies. It's actually the correct proxy behavior which we just couldn't achieve before: no execution of any constructor or other user code in the CGLIB proxy instance itself. Phil, with our Objenesis 1.3 vs 1.0 detection in the test suite, aren't we seeing such conflicts already? With Objenesis 2.0, there could be serious conflicts with Mockito and co. I'm still leaning towards embedding it if we support it at all. An option to switch it off is fine - we can do this against an inlined version as well. That might be the biggest concern with Oliver's pull request, actually: It simply uses Objenesis when found on the classpath. If a user is not aware of the exact contents of his classpaths, he may be seeing strange differences at runtime. I'd rather opt for consistent behavior in Spring 4.0, controlled by our configuration only. Juergen |
Juergen Hoeller commented We could also turn it around and only use Objenesis when explicitly activated. I would still prefer an embedded version even then, relying on an explicit configuration flag instead of presence of an arbitrary Objenesis version in the classpath. The IBM JVM is noticeably absent in the list of supported VMs. That might mean we have to turn Objenesis off by default, for consistent behavior (i.e. consistent execution/non-execution of proxy constructors) across JVMs by default. Juergen |
Fedor Bobin commented
Looks reasonable.
Objenesis 1.4 and 2.0 uses Unsafe.allocateInstance as default for unknown JVM. So IBM JVM is supported. |
Juergen Hoeller commented Indeed, sun.misc.Unsafe is generally known to work fine on IBM JVMs. Maybe we can use Objenesis by default then - it only applies to CGLIB proxies anyway -, and just have a setting to switch it off. We're doing a 4.0, after all; taking some risk is allowed :-) Juergen |
Fedor Bobin opened SPR-10594 and commented
There is some limitations in current proxy instantiation model:
Note that proxy does not need to have any valid state at all.
Solution is to use Objenesis library ( http://objenesis.googlecode.com/svn/docs/index.html ). It can instantiate objects without calling any constructor and cglib-callbacks can be set after creation.
If Objenesis library is in classpath => create using it.
If no Objenesis library is in classpath => create using default constructor (fallback to current implementation).
So user that does not want to have Objenesis dependency in class path will have old instantiation model.
You can see proposed changes at #290
Additionaly there is a problem with proxy around Externalizable classes: this proxies are not serialized correctly. Solution is to make proxies implement java.io.Externalizable and add read/writeExternal implementations to them. It was done for proxies created by Objenesis. Seems that it should be done also for proxies created using default constructor.
Reference URL: #290
Issue Links:
1 votes, 6 watchers
The text was updated successfully, but these errors were encountered: