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.

Sign up for GitHub

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

Jump to bottom

[OPENJPA-2817] dropping _transforming from PCClassFileTransformer and replace it by a minimal exclusion list #65

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

[OPENJPA-2817] dropping _transforming from PCClassFileTransformer and replace it by a minimal exclusion list #65

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd70273
OPENJPA-2817 dropping _transforming from PCClassFileTransformer and r…
rmannibucau Jul 14, 2020
1c14bc2
excluding asm too
rmannibucau Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 54 additions & 25 deletions openjpa-kernel/src/main/java/org/apache/openjpa/enhance/PCClassFileTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public class PCClassFileTransformer
private final ClassLoader _tmpLoader;
private final Log _log;
private final Set _names;
private boolean _transforming = false;

/**
* Constructor.
Expand All @@ -64,7 +63,8 @@ public class PCClassFileTransformer
* @param opts enhancer configuration options
* @param loader temporary class loader for loading intermediate classes
*/
public PCClassFileTransformer(MetaDataRepository repos, Options opts, ClassLoader loader) {
public PCClassFileTransformer(MetaDataRepository repos, Options opts,
ClassLoader loader) {
this(repos, toFlags(opts), loader, opts.removeBooleanProperty
("scanDevPath", "ScanDevPath", false));
}
Expand Down Expand Up @@ -105,6 +105,9 @@ public PCClassFileTransformer(MetaDataRepository repos, PCEnhancer.Flags flags,
_log.info(_loc.get("runtime-enhance-pcclasses"));
}

// this must be called under a proper locking, this is guaranteed by either
// a correct concurrent classloader with transformation support OR
// a mono-threaded access guarantee (build, deploy time enhancements).
@Override
public byte[] transform(ClassLoader loader, String className, Class redef, ProtectionDomain domain, byte[] bytes)
throws IllegalClassFormatException {
Expand All @@ -116,17 +119,44 @@ public byte[] transform(ClassLoader loader, String className, Class redef, Prote
if (className == null) {
return null;
}
// prevent re-entrant calls, which can occur if the enhancing
// loader is used to also load OpenJPA libraries; this is to prevent
// recursive enhancement attempts for internal openjpa libraries
if (_transforming)
return null;

_transforming = true;

return transform0(className, redef, bytes);
}

// very simplified flavor of
// @apache/tomee >
// container/openejb-core >
// org/apache/openejb/util/classloader/URLClassLoaderFirst.java#L207
private boolean isExcluded(final String className) {
if ( // api
className.startsWith("javax/") ||
className.startsWith("jakarta/") ||
// openjpa dependencies
className.startsWith("serp/") ||
// jvm
className.startsWith("java/") ||
className.startsWith("sun/") ||
className.startsWith("jdk/")) {
return true;
}
// it is faster to use substring when multiple tests are needed
if (className.startsWith("org/apache/")) {
final String sub = className.substring("org/apache/".length());
if (sub.startsWith("openjpa/") ||
Copy link
Member

Choose a reason for hiding this comment

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

this would likely trash our test classes too? They as well start with org.apache.openjpa...

Copy link
Contributor

@dazey3 dazey3 Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I personally don't like code changes like this. Referencing hardcoded strings and doing numerous string comparisons/manipulation is kinda gross/hacky. All this complex string comparison is going to be pretty bad for performance when you have 100 persistence classes to transform and the have to be checked repeatedly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@struberg yes but it works cause we use build time enhancement - also probably why we didn't notice this issue earlier
@dazey3 as mentionned please benchmark it on the different kind of mentionned apps (no deps - jpa in the container, no deps + no tempclassloader - means temploader=apploader, lot of deps, lot of deps without temp loader, a few deps, a few deps without temp loader and you do the test without the javaagent or with it for all these cases). In practise this change is faster than most temploader usage and most bytecode reading so the assumption it is slower is not justified in the general case.

sub.startsWith("commons/") ||
sub.startsWith("xbean/")) {
return true;
}
}
if (className.startsWith("com/")) {
final String sub = className.substring("com/".length());
if (sub.startsWith("oracle/") || sub.startsWith("sun/")) { // jvm
return true;
}
}
return false;
}

/**
* We have to split the transform method into two methods to avoid
* ClassCircularityError when executing method using pure-JIT JVMs
Expand All @@ -149,7 +179,7 @@ private byte[] transform0(String className, Class redef, byte[] bytes)
try {
PCEnhancer enhancer = new PCEnhancer(_repos.getConfiguration(),
new Project().loadClass(new ByteArrayInputStream(bytes),
_tmpLoader), _repos);
_tmpLoader), _repos, _tmpLoader);
enhancer.setAddDefaultConstructor(_flags.addDefaultConstructor);
enhancer.setEnforcePropertyRestrictions
(_flags.enforcePropertyRestrictions);
Expand All @@ -170,7 +200,6 @@ private byte[] transform0(String className, Class redef, byte[] bytes)
throw (IllegalClassFormatException) t;
throw new GeneralException(t);
} finally {
_transforming = false;
if (returnBytes != null && _log.isTraceEnabled())
_log.trace(_loc.get("runtime-enhance-complete", className,
bytes.length, returnBytes.length));
Expand All @@ -180,41 +209,41 @@ private byte[] transform0(String className, Class redef, byte[] bytes)
/**
* Return whether the given class needs enhancement.
*/
private Boolean needsEnhance(String clsName, Class redef, byte[] bytes) {
if (redef != null && PersistenceCapable.class.isAssignableFrom(redef)) {
private Boolean needsEnhance(String clsName, Class<?> redef, byte[] bytes) {
final boolean excluded = isExcluded(clsName);

if (!excluded && redef != null && PersistenceCapable.class.isAssignableFrom(redef)) {
// if the original class is already enhanced (implements PersistenceCapable)
// then we don't need to do any further processing.
return null;
}

if (_names != null) {
if (_names.contains(clsName.replace('/', '.')))
return Boolean.valueOf(!isEnhanced(bytes));
return null;
}

if (clsName.startsWith("java/") || clsName.startsWith("javax/") || clsName.startsWith("jakarta/")) {
return !isEnhanced(bytes);
if (excluded) {
return false;
}
return null;
}

if (isEnhanced(bytes)) {
return Boolean.FALSE;
}
if (excluded || isEnhanced(bytes))
return false;

try {
Class c = Class.forName(clsName.replace('/', '.'), false,
Class<?> c = Class.forName(clsName.replace('/', '.'), false,
_tmpLoader);
if (_repos.getMetaData(c, null, false) != null)
return Boolean.TRUE;
return true;
return null;
} catch (ClassNotFoundException cnfe) {
// cannot load the class: this might mean that it is a proxy
// or otherwise inaccessible class which can't be an entity
return Boolean.FALSE;
return false;
} catch (LinkageError cce) {
// this can happen if we are loading classes that this
// class depends on; these will never be enhanced anyway
return Boolean.FALSE;
return false;
} catch (RuntimeException re) {
throw re;
} catch (Throwable t) {
Expand Down