Skip to content
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

Use target ClassLoader in ClassWriter implementation to find classes #118

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Lerm
Copy link

@Lerm Lerm commented Mar 12, 2018

Proposed fix for #117 issue.

Cglib 3.2+ specifies COMPUTE_FRAMES flag during creation of the ClassWriter object to enforce recalculation of stack map frames (which is probably the right thing to do), which causes invocation of ClassWriter.getCommonSuperClass method during incoming stack frames merging. Default ClassWriter implementation uses current class-loader (i.e. classloader which was used to load ClassWriter itself) to load classes of merging types – so, if target class is unavailable in this class-loader, then the code generation fails. This problem can be observed in any configuration with multi-level class loader configuration - one example is JEE environment in which cglib itself is located on EAR application level, but is used to instrument classes in children WAR-modules (which usually have their own classloaders under EAR to allow hot reloading of classes).

In order to fix this problem, we can override the 'getCommonSuperClass' method of the ClassWriter class and load classes using the same classloader which is used to define the resulting generated class (i.e. Enhancer.getClassLoader – which is either explicitly defined or derived from super-class/interfaces). This PR includes both the proposed fix and unit tests for both Enhancer and TransformingClassLoader cases.

Copy link
Contributor

@vlsi vlsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for the change

@@ -0,0 +1,62 @@
/*
* Copyright 2003,2004 The Apache Software Foundation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 2018?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting question. This particular code wasn't actually developed by Apache Foundation, so it's technically incorrect to include its license header here - so it seems, that it's better just to delete it.

@@ -0,0 +1,119 @@
package net.sf.cglib.classloader;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is copyright header missing here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular code wasn't actually developed by Apache Foundation, so it's technically incorrect to include its copyright header here. The cglib process itself has license and notice files in the project structure, so there is no need to include copyright header in every file, I assume. However, if there is any defined file-header template for the project - it can be applied, but I haven't found any.

}
}
} else {
return super.loadClass(name, resolve);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to reorder if branch to avoid if (...) { long code ... long code} else {return} pattern

@@ -0,0 +1,115 @@
package net.sf.cglib.classloader;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright header?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular code wasn't actually developed by Apache Foundation, so it's technically incorrect to include its copyright header here. The cglib process itself has license and notice files in the project structure, so there is no need to include copyright header in every file, I assume. However, if there is any defined file-header template for the project - it can be applied, but I haven't found any.

@raphw
Copy link
Member

raphw commented Mar 12, 2018

Yes, good call. We need to do a new release with ASM 6.1 anyways, great if you can get the comments fixed then I will happily merge it. I think that is a good way to approach this without changing default behavior.

@Lerm
Copy link
Author

Lerm commented Mar 20, 2018

Thank you for the comments! I've made changes according to them - except for the license header, as I don't know whether any particular header need to applied to new files in this project. Is there any other changes you think are required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants