Fix permgen leak by storing Class instances as String #50
Conversation
| @@ -84,14 +84,30 @@ | |||
| 938313161, 1288102441, 1768288259 }; | |||
|
|
|||
|
|
|||
| public static final Customizer CLASS_BY_NAME = new Customizer() { | |||
| public static final Customizer CLASS_BY_NAME = new Customizer2() { | |||
| public void customize(CodeEmitter e, Type type) { | |||
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
| @@ -425,7 +425,7 @@ private static void hash_object(CodeEmitter e, Type type, Customizer customizer) | |||
| Label end = e.make_label(); | |||
| e.dup(); | |||
| e.ifnull(skip); | |||
| if (customizer != null) { | |||
| if (customizer != null && !(customizer instanceof FieldTypeCustomizer)) { | |||
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.
vlsi
force-pushed
the
fix_class_leak
branch
2 times, most recently
from
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 [email protected]
|
|
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.