modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientFactory.java | 13 ++- modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientProxy.java | 10 ++ modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ClassPoolFactory.java | 34 ++++------ modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ConfigurationClassBuilder.java | 24 +++---- modules/enterprise/binding/src/main/java/org/rhq/bindings/util/InterfaceSimplifier.java | 3 5 files changed, 42 insertions(+), 42 deletions(-)

New commits: commit 99bbd52d13b16fe57c80c6dab7230942cfd49560 Author: Lukas Krejci lkrejci@redhat.com Date: Fri Mar 29 17:04:21 2013 +0100

[BZ 928971] - ClassPoolFactory is now resilient against context classloader changes

The instance must not be cached because it could be using a wrong context classloader if that changed between invocations.

Also made changes to lower the numbder of instances of the ClassPool in the created while using the ConfigurationClassBuilder.

This, in addition to commit fe8952cf9631c2d839f9e37d53f8c1aa1bb04b83 (which I failed to annotate with the BZ number), should fix the bug.

diff --git a/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientFactory.java b/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientFactory.java index 2e6ce5c..63c96f0 100644 --- a/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientFactory.java +++ b/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientFactory.java @@ -4,7 +4,6 @@ import java.io.PrintWriter; import java.lang.reflect.InvocationTargetException; import java.security.AccessController; import java.security.PrivilegedAction; -import java.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -153,7 +152,7 @@ public class ResourceClientFactory { private Class<?> defineCustomInterface(ResourceClientProxy proxy) { try { // define the dynamic class - do not put it in any known rhq package in case our jars are signed (see BZ-794503) - ClassPool pool = ClassPoolFactory.get(); + ClassPool pool = ClassPoolFactory.newInstance(); CtClass customClass = pool.makeInterface("org.rhq.bindings.client.dynamic." + ResourceClientProxy.class.getSimpleName() + proxy.fingerprint);

@@ -173,8 +172,8 @@ public class ResourceClientFactory { } else if (prop instanceof ResourceClientProxy.Operation) { ResourceClientProxy.Operation o = (ResourceClientProxy.Operation) prop;

- LinkedHashMap<String, CtClass> types = ConfigurationClassBuilder.translateParameters(o - .getDefinition().getParametersConfigurationDefinition()); + LinkedHashMap<String, CtClass> types = ConfigurationClassBuilder.translateParameters(pool, o + .getDefinition().getParametersConfigurationDefinition());

CtClass[] params = new CtClass[types.size()]; int x = 0; @@ -182,8 +181,10 @@ public class ResourceClientFactory { params[x++] = types.get(param); }

- CtMethod method = CtNewMethod.abstractMethod(ConfigurationClassBuilder.translateConfiguration(o - .getDefinition().getResultsConfigurationDefinition()), ResourceClientProxy.simpleName(key), params, + CtMethod method = CtNewMethod + .abstractMethod(ConfigurationClassBuilder.translateConfiguration(pool, o + .getDefinition().getResultsConfigurationDefinition()), ResourceClientProxy.simpleName(key), + params, new javassist.CtClass[0], customClass);

// Setup @WebParam annotations so the signatures have the config prop names diff --git a/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientProxy.java b/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientProxy.java index 9b20aef..fc5e610 100644 --- a/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientProxy.java +++ b/modules/enterprise/binding/src/main/java/org/rhq/bindings/client/ResourceClientProxy.java @@ -29,11 +29,13 @@ import java.util.List; import java.util.Map; import java.util.Set;

+import javassist.ClassPool; import javassist.util.proxy.MethodHandler;

import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory;

+import org.rhq.bindings.util.ClassPoolFactory; import org.rhq.bindings.util.ConfigurationClassBuilder; import org.rhq.bindings.util.LazyLoadScenario; import org.rhq.bindings.util.ResourceTypeFingerprint; @@ -410,7 +412,9 @@ public class ResourceClientProxy { if (!LazyLoadScenario.isShouldLoad()) return null;

- Configuration parameters = ConfigurationClassBuilder.translateParametersToConfig(definition + ClassPool pool = ClassPoolFactory.newInstance(); + + Configuration parameters = ConfigurationClassBuilder.translateParametersToConfig(pool, definition .getParametersConfigurationDefinition(), args);

OperationManagerRemote operationManager = remoteClient.getProxy(OperationManagerRemote.class); @@ -441,7 +445,7 @@ public class ResourceClientProxy {

Configuration result = (history != null ? history.getResults() : null);

- Object returnResults = ConfigurationClassBuilder.translateResults(definition + Object returnResults = ConfigurationClassBuilder.translateResults(pool, definition .getResultsConfigurationDefinition(), result);

return returnResults; @@ -525,7 +529,7 @@ public class ResourceClientProxy { /** * @deprecated Superseded by ({@link #updateBackingContent(String, String)} * - * @param fileName file name + * @param filename file name */ @Deprecated public void updateBackingContent(String filename) { diff --git a/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ClassPoolFactory.java b/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ClassPoolFactory.java index 50673e8..38397db 100644 --- a/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ClassPoolFactory.java +++ b/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ClassPoolFactory.java @@ -28,24 +28,22 @@ import javassist.ClassPool; import javassist.LoaderClassPath;

/** - * This class is used to create Javassist's classpools usable in RHQ on both client and server side. - * This only exists to centralize the initialization code for the pools. - * <p> - * This class is similar to how the <code>ClassPool.getDefault()</code> method operates (in that it only ever returns - * a single instance of the pool) but uses a "wider" class path, looking for classes using the context class loader - * and the using the default resource lookup of the <code>Class</code> class, in addition to just the system class path - * as detected by Javassist (which is the only one used in the class pool returned by - * <code>ClassPool.getDefault()</code>). - * <p> - * This is to ensure that Javassist can locate the classes in various classloading "schemes" - the traditional + * This class is used to create Javassist's classpools usable in RHQ on both client and server side. This only exists to + * centralize the initialization code for the pools. + * + * <p> Unlike the <code>ClassPool.getDefault()</code> method that only ever returns a single instance of the pool, this + * factory returns a <b>new instance</b> every time. This is because it uses a "wider" class path, looking for classes + * using the current thread context class loader but also using the default resource lookup of the <code>Class</code> + * class, in addition to just the system class path as detected by Javassist (which is the only one used in the class + * pool returned by <code>ClassPool.getDefault()</code>). + * + * <p> This is to ensure that Javassist can locate the classes in various classloading "schemes" - the traditional * application classloader used in the CLI client and the JBoss Modules classloading used on the server. * * @author Lukas Krejci */ public class ClassPoolFactory {

- private static ClassPool pool; - private ClassPoolFactory() {

} @@ -53,13 +51,11 @@ public class ClassPoolFactory { /** * @return the singleton class pool instance initialized according the {@link ClassPoolFactory rules}. */ - public static synchronized ClassPool get() { - if (pool == null) { - pool = new ClassPool(null); - pool.appendClassPath(new LoaderClassPath(Thread.currentThread().getContextClassLoader())); - pool.appendClassPath(new ClassClassPath(ClassPoolFactory.class)); - pool.appendSystemPath(); - } + public static ClassPool newInstance() { + ClassPool pool = new ClassPool(null); + pool.appendClassPath(new LoaderClassPath(Thread.currentThread().getContextClassLoader())); + pool.appendClassPath(new ClassClassPath(ClassPoolFactory.class)); + pool.appendSystemPath();

return pool; } diff --git a/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ConfigurationClassBuilder.java b/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ConfigurationClassBuilder.java index fd72443..d01ce14 100644 --- a/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ConfigurationClassBuilder.java +++ b/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/ConfigurationClassBuilder.java @@ -40,7 +40,7 @@ public class ConfigurationClassBuilder { * @return Map of propertyName to types for the config def * @throws NotFoundException */ - public static LinkedHashMap<String, CtClass> translateParameters(ConfigurationDefinition def) + public static LinkedHashMap<String, CtClass> translateParameters(ClassPool cp, ConfigurationDefinition def) throws NotFoundException { LinkedHashMap<String, CtClass> result = new LinkedHashMap<String, CtClass>(); if (def == null || def.getPropertyDefinitions() == null) { @@ -51,14 +51,14 @@ public class ConfigurationClassBuilder { if (pd instanceof PropertyDefinitionSimple) { PropertyDefinitionSimple simple = (PropertyDefinitionSimple) pd; String name = pd.getName(); - CtClass paramType = getSimpleTypeClass(simple); + CtClass paramType = getSimpleTypeClass(cp, simple); result.put(name, paramType); } } return result; }

- private static CtClass getSimpleTypeClass(PropertyDefinitionSimple simple) throws NotFoundException { + private static CtClass getSimpleTypeClass(ClassPool cp, PropertyDefinitionSimple simple) throws NotFoundException { Class<?> paramType = null; switch (simple.getType()) { case STRING: @@ -84,19 +84,19 @@ public class ConfigurationClassBuilder { paramType = Double.TYPE; break; } - return ClassPoolFactory.get().get(paramType.getName()); + return cp.get(paramType.getName()); }

- public static CtClass translateConfiguration(ConfigurationDefinition def) throws NotFoundException { + public static CtClass translateConfiguration(ClassPool cp, ConfigurationDefinition def) throws NotFoundException { if (def == null) { return CtClass.voidType; } else if (def.getPropertyDefinitionSimple("operationResult") != null) { // Its a simple type - return getSimpleTypeClass(def.getPropertyDefinitionSimple("operationResult")); + return getSimpleTypeClass(cp, def.getPropertyDefinitionSimple("operationResult")); } else {

// TODO GH: Build a custom type? - return ClassPoolFactory.get().get(Configuration.class.getName()); + return cp.get(Configuration.class.getName()); } }

@@ -108,9 +108,9 @@ public class ConfigurationClassBuilder { return Character.toLowerCase(name.charAt(0)) + name.substring(1, name.length()); }

- public static Configuration translateParametersToConfig(ConfigurationDefinition parametersConfigurationDefinition, + public static Configuration translateParametersToConfig(ClassPool cp, ConfigurationDefinition parametersConfigurationDefinition, Object[] args) throws NotFoundException { - LinkedHashMap<String, CtClass> translateParameters = translateParameters(parametersConfigurationDefinition); + LinkedHashMap<String, CtClass> translateParameters = translateParameters(cp, parametersConfigurationDefinition); Configuration config = new Configuration();

int index = 0; @@ -122,12 +122,12 @@ public class ConfigurationClassBuilder {

}

- public static Object translateResults(ConfigurationDefinition resultsConfigurationDefinition, Configuration result) + public static Object translateResults(ClassPool cp, ConfigurationDefinition resultsConfigurationDefinition, Configuration result) throws NotFoundException {

- CtClass expectedReturn = translateConfiguration(resultsConfigurationDefinition); + CtClass expectedReturn = translateConfiguration(cp, resultsConfigurationDefinition);

- if (expectedReturn.equals(ClassPoolFactory.get().get(Configuration.class.getName()))) { + if (expectedReturn.equals(ClassPoolFactory.newInstance().get(Configuration.class.getName()))) { return result; } else { //bail on translation if Configuration passed in is null diff --git a/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/InterfaceSimplifier.java b/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/InterfaceSimplifier.java index 1e13179..640f8bf 100644 --- a/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/InterfaceSimplifier.java +++ b/modules/enterprise/binding/src/main/java/org/rhq/bindings/util/InterfaceSimplifier.java @@ -22,7 +22,6 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List;

-import javassist.CannotCompileException; import javassist.ClassPool; import javassist.CtClass; import javassist.CtMethod; @@ -143,7 +142,7 @@ public class InterfaceSimplifier {

public static Class<?> simplify(Class<?> intf) { try { - ClassPool classPool = ClassPoolFactory.get(); + ClassPool classPool = ClassPoolFactory.newInstance();

String simplifiedName = getSimplifiedName(intf); LOG.debug("Simplifying " + intf + " (simplified interface name: " + simplifiedName + ")...");