Fix permgen leak by storing Class instances as String#50
Conversation
There was a problem hiding this comment.
This should be a no-op and be defined in the interface
There was a problem hiding this comment.
Technically speaking, if someone uses CLASS_BY_NAME as Customizer, he/she would expect Customizer.customize(CodeEmitter e, Type type) do some meaningful stuff.
So, the implementation is there for backward compatibility
There was a problem hiding this comment.
Can these logic be handled by the concrete Customizer rather than here?
There was a problem hiding this comment.
I can do that, however it would require adding additional field for fieldCustomizer.
The thing is logic behind old and new customizers is quite different: old customizer augmented hashCode/equals code while new one augments field types and constructor.
83289d1 to
6a9873a
Compare
|
Please check updated PR. |
Previously KeyFactory generated java.lang.Class fields even with CLASS_BY_NAME customization. That might lead to unexpected class and classloader leak. The fix ensures KeyFactory uses java.lang.String to store class names. It should make no harm as CGLIB uses per-classloader cache, thus class name should be sufficiently unique
|
@sameb Any chance this can be merged? How long would it take to have a release in the maven repos that includes this fix? |
|
Is there anybody else apart from @sameb that can merge PRs? |
|
@JBodkin, @lukesandberg, can you have a look? |
|
Sorry I don't have commiter privileges. On Tue, Dec 22, 2015, 7:41 AM Vladimir Sitnikov notifications@github.com
|
|
I'm afraid I don't have committer privileges, hopefully sameb will be back after the holidays. |
|
Hi everyone, I'm going to talk with Chris N (one of the original owners of cglib) about adding more committers here, since I don't have the time to keep up with all the pull requests. I'll try to review the outstanding ones over the course of this week. |
To the best of my knowledge, this looks like a good solution given the bounds of the current implementation. I favour this solution over #54, it seems like the cleaner solution and it is benchmarked.
Previously
KeyFactorygeneratedjava.lang.Classfields even withCLASS_BY_NAMEcustomization.That might lead to unexpected class and classloader leak.
The fix ensures
KeyFactoryusesjava.lang.Stringto store class names.It should make no harm as CGLIB uses per-classloader cache, thus class name should be sufficiently unique.