diff --git a/springloaded/notes.md b/springloaded/notes.md index 9f6248b..b3aa627 100644 --- a/springloaded/notes.md +++ b/springloaded/notes.md @@ -1,10 +1,28 @@ +Implementation details: + +catchers +- A catcher is added to a reloadable type as it is being loaded for the first time. A reloadable type gets +a catcher for each method it inherits from a parent but does not override, or for an abstract class each +method it receives from an interface. They basically stand in for methods that could be added in later +reloads. The catchers 'catch' virtual method dispatches. Their job is either to (a) make a simple super +call if the reloadable type does not implement the specified method (yet), or (b) if the method +has been implemented by a reload, forward to the new implementation. Catchers are not created for +private, final or static methods since those cannot be overridden. If you modify a final method in a reloadable +type to make it non final and then override it, it will be handled in a different way than catchers. + +superdispatchers +- Where a method is protected in a non-reloadable type it is necessary to add a superdispatcher method to the +subtype so that when the executor for a new version is running it can access that protected method. The +superdispatcher is simply a public method on a type that calls super. + Helpful snippets when debugging tests: ClassPrinter.print(z.getLatestExecutorBytes()); Utils.dump("foo/SubControllerB", rtype.bytesLoaded); - + + \ No newline at end of file diff --git a/springloaded/src/main/java/org/springsource/loaded/AbstractMember.java b/springloaded/src/main/java/org/springsource/loaded/AbstractMember.java index 024c385..e3e20f1 100644 --- a/springloaded/src/main/java/org/springsource/loaded/AbstractMember.java +++ b/springloaded/src/main/java/org/springsource/loaded/AbstractMember.java @@ -109,7 +109,7 @@ public abstract class AbstractMember implements Constants { return Modifier.isPublic(getModifiers()); } - public boolean isPrivateStaticFinal() { + public boolean isPrivateOrStaticOrFinal() { return (modifiers & ACC_PRIVATE_STATIC_FINAL) != 0; } diff --git a/springloaded/src/main/java/org/springsource/loaded/TypeDescriptorExtractor.java b/springloaded/src/main/java/org/springsource/loaded/TypeDescriptorExtractor.java index 95b5c85..2c37f53 100644 --- a/springloaded/src/main/java/org/springsource/loaded/TypeDescriptorExtractor.java +++ b/springloaded/src/main/java/org/springsource/loaded/TypeDescriptorExtractor.java @@ -36,6 +36,8 @@ import org.objectweb.asm.Opcodes; */ public class TypeDescriptorExtractor { + private final static boolean DEBUG_TYPE_DESCRIPTOR_EXTRACTOR = false; + private TypeRegistry registry; public TypeDescriptorExtractor(TypeRegistry registry) { @@ -50,7 +52,7 @@ public class TypeDescriptorExtractor { } /** - * Visit a class and accumulate enough information to build a TypeDescriptor. + * Visit a class and accumulate sufficient information to build a TypeDescriptor. */ class ExtractionVisitor implements ClassVisitor, Opcodes { @@ -60,7 +62,9 @@ public class TypeDescriptorExtractor { private String superclassName; private String[] interfaceNames; private boolean isGroovy = false; + private boolean isEnum = false; private boolean hasClinit = false; + // TODO [perf - reduce garbage] make these collections lazily initialize private List constructors = new ArrayList(); private List methods = new ArrayList(); private List fieldsRequiringAccessors = new ArrayList(); @@ -71,7 +75,6 @@ public class TypeDescriptorExtractor { this.isReloadableType = isReloadableType; } - public TypeDescriptor getTypeDescriptor() { if (isReloadableType) { computeCatchersAndSuperdispatchers(); @@ -87,7 +90,7 @@ public class TypeDescriptorExtractor { } /** - * Determine if there are clashes. A clash is where a static method takes the this reloadable type as its first parameter + * Determine if there are clashes. A clash is where a static method takes the reloadable type as its first parameter * but in all other ways is the same as an existing instance method. For example this instance method A.foo(String) clashes * with this static method A.foo(A, String). 'clashing' means the executor will have to do something to avoid a duplicate * method problem and we'll have to differentiate between the two. @@ -120,6 +123,7 @@ public class TypeDescriptorExtractor { return registry.getDescriptorFor(slashedname); } + // TODO [refactor] extract the type registry relationship code into a central helper class private TypeDescriptor findTypeDescriptor(TypeRegistry registry, String typename) { // follow the pattern for a classloader: recurse up trying to find it, then recurse down trying to load it TypeRegistry regToTry = registry; @@ -138,77 +142,92 @@ public class TypeDescriptorExtractor { } /** - * Create catcher methods for methods from our super-hierarchy that we don't yet override (but may after the initial define - * has happened). + * Algorithm: Go up the superclass hierarchy for a type and determine what should be caught in this type (see + * 'catchers' in notes.md). Methods that are private, static or final do *not* get a catcher. This method + * also computes superdispatchers - see 'superdispatchers' in notes.md + * + */ + private void walkHierarchyForCatchersAndSuperDispatchers(String superclass, List superDispatchers, List finalInHierarchy) { + TypeDescriptor supertypeDescriptor = superclass==null?null:findTypeDescriptor(registry, superclass); + if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) { + System.out.println("Computing catchers on "+this.typename+" from superclass "+superclass); + } + boolean isReloadable = supertypeDescriptor.isReloadable(); + for (MethodMember method: supertypeDescriptor.getMethods()) { + if (shouldCreateSuperDispatcherFor(method) && !superDispatchers.contains(method.nameAndDescriptor)) { + // need a public super dispatcher - so that we can reach that super method + // from a reloaded instance of this type + MethodMember superdispatcher = method.superDispatcherFor(); + methods.add(superdispatcher); + superDispatchers.add(method.nameAndDescriptor); + } + if (shouldCatchMethod(method) && !finalInHierarchy.contains(method.getNameAndDescriptor())) { + // don't need the catcher if method is already defined since when the existing method is rewritten + // it will be kind of morphed into a catcher + // TODO what about a private method that is overridden by a static method (same name/descriptor but not + // an overrides relationship) + if (!isReloadable && Modifier.isFinal(method.getModifiers())) { + // Do not create a catcher, the supertype is not reloadable and so an implementation cannot be + // added lower in the type hierarchy + finalInHierarchy.add(method.getNameAndDescriptor()); + continue; + } + MethodMember found = null; + for (MethodMember existingMethod : methods) { + if (existingMethod.equalsApartFromModifiers(method)) { + found = existingMethod; + break; + } + } + if (found != null) { + continue; + } + MethodMember catcherCopy = method.catcherCopyOf(); + if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) { + System.out.println("Adding catcher for "+method.nameAndDescriptor); + } + methods.add(catcherCopy); + } else { + if (method.isFinal()) { + finalInHierarchy.add(method.getNameAndDescriptor()); + } + } + } + if (supertypeDescriptor.supertypeName != null) { + walkHierarchyForCatchersAndSuperDispatchers(supertypeDescriptor.supertypeName, superDispatchers, finalInHierarchy); + } + if (Modifier.isAbstract(this.flags) && !this.isEnum/* && !Modifier.isInterface(this.flags)*/) { + // abstract class may be missing methods that it can implement from the interfaces + for (String interfaceName : supertypeDescriptor.superinterfaceNames) { + addCatchersForNonImplementedMethodsFrom(interfaceName, finalInHierarchy); + } + } + } + + /** + * Compute and add the catch methods and super dispatch methods that apply to this type. */ private void computeCatchersAndSuperdispatchers() { - // When walking up the hierarchy we may hit a 'final' method which means we must not catch it. - // The 'shouldNotCatch' list stores things we discover like this that should not be caught - List shouldNotCatch = new ArrayList(); - - String type = superclassName; - // Don't need catchers in interfaces - if (Modifier.isInterface(this.flags)) { + if (Modifier.isInterface(this.flags)) { // Don't need catchers in interfaces return; } - List superDispatcherAddedFor = new ArrayList(); - while (type != null) { - TypeDescriptor supertypeDescriptor = findTypeDescriptor(registry, type); - // TODO review the need to create catchers for methods where the supertype is reloadable. In this situation we are already going to - // be intercepting the call side of these methods so we don't need the catcher. Could be a large performance increase and reduction in - // permgen, and simplification of stack traces - // if (!supertypeDescriptor.isReloadable()) { - for (MethodMember method : supertypeDescriptor.getMethods()) { - if (shouldCreateSuperDispatcherFor(method) && !superDispatcherAddedFor.contains(method.nameAndDescriptor)) { - // need a public super dispatcher - so that we can reach that super method - // from a reloaded instance of this type - MethodMember superdispatcher = method.superDispatcherFor(); - methods.add(superdispatcher); - superDispatcherAddedFor.add(method.nameAndDescriptor); - } - - if (shouldCatchMethod(method) && !shouldNotCatch.contains(method.getNameAndDescriptor())) { - // don't need the catcher if method is already defined since when the existing method is rewritten - // it will be kind of morphed into a catcher - // TODO what about a private method that is overridden by a static method (same name/descriptor but not - // an overrides relationship) - // if (supertypeDescriptor.isGroovyType() && !isGroovy) { - // if (method.getName().startsWith("super$")) { - // continue; - // } - // } - MethodMember found = null; - for (MethodMember existingMethod : methods) { - if (existingMethod.equalsApartFromModifiers(method)) { - found = existingMethod; - break; - } - } - if (found != null) { - continue; - } - MethodMember catcherCopy = method.catcherCopyOf(); - // System.out.println("catcher is " + catcherCopy + " is groovy type? " + this.isGroovy); - methods.add(catcherCopy); - } else { - if (method.isFinal()) { - shouldNotCatch.add(method.getNameAndDescriptor()); - } - } - } - // } - type = supertypeDescriptor.supertypeName; - } + + // TODO [review design] review the need to create catchers for methods where the supertype is reloadable. + // Can we just add them to the topmost reloadable type? + List doNotCatch = new ArrayList(); + walkHierarchyForCatchersAndSuperDispatchers(superclassName, new ArrayList(), doNotCatch); - // ought to look in interfaces *if* we are an abstract class - if (Modifier.isAbstract(this.flags)/* && !Modifier.isInterface(this.flags)*/) { - // abstract class + // ought to look in interfaces if we are an abstract class + if (Modifier.isAbstract(this.flags) && !this.isEnum/* && !Modifier.isInterface(this.flags)*/) { + // abstract class may be missing methods that it can implement from the interfaces for (String interfaceName : interfaceNames) { - addCatchersForNonImplementedMethodsFrom(interfaceName); + addCatchersForNonImplementedMethodsFrom(interfaceName, doNotCatch); } } - - finalInHierarchy.addAll(shouldNotCatch); + if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) { + System.out.println("For "+this.typename+" setting finalsInHierarchy to "+doNotCatch); + } + finalInHierarchy.addAll(doNotCatch); } // TODO should clone and finalize be in here? @@ -218,8 +237,7 @@ public class TypeDescriptorExtractor { (method.getName().equals("clone") && method.getDescriptor().equals("()Ljava/lang/Object;"))); } - - private void addCatchersForNonImplementedMethodsFrom(String interfacename) { + private void addCatchersForNonImplementedMethodsFrom(String interfacename,List finalInNonReloadableType) { TypeDescriptor interfaceDescriptor = findTypeDescriptor(registry, interfacename); for (MethodMember method : interfaceDescriptor.getMethods()) { // If this class doesn't implement this interface method, add it @@ -230,17 +248,21 @@ public class TypeDescriptorExtractor { break; } } - if (!found) { + if (!found && !finalInNonReloadableType.contains(method.getNameAndDescriptor())) { + if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) { + Log.log("adding catcher for ["+method+"] from ["+interfacename+"] to ["+this.typename+"]"); + } methods.add(method.catcherCopyOfWithAbstractRemoved()); } } for (String interfaceName : interfaceDescriptor.superinterfaceNames) { - addCatchersForNonImplementedMethodsFrom(interfaceName); + addCatchersForNonImplementedMethodsFrom(interfaceName,finalInNonReloadableType); } } /** - * Field + * Protected fields in reloadable parents of a class need an accessor adding to the reloadable + * type so that the fields can be reached from the executor. */ private void computeFieldsRequiringAccessors() { String type = superclassName; @@ -274,13 +296,17 @@ public class TypeDescriptorExtractor { * @return true if it should be caught */ private boolean shouldCatchMethod(MethodMember method) { - return !(method.isPrivateStaticFinal() || method.getName().endsWith(Constants.methodSuffixSuperDispatcher) || (method.getName().equals("finalize") && method.getDescriptor().equals("()V"))); + return !(method.isPrivateOrStaticOrFinal() || method.getName().endsWith(Constants.methodSuffixSuperDispatcher) || + (method.getName().equals("finalize") && method.getDescriptor().equals("()V"))); } public void visit(int version, int flags, String name, String signature, String superclassName, String[] interfaceNames) { this.flags = flags; this.superclassName = superclassName; this.interfaceNames = interfaceNames; + if (superclassName!=null && superclassName.equals("java/lang/Enum")) { + this.isEnum = true; + } this.typename = name; } @@ -304,7 +330,7 @@ public class TypeDescriptorExtractor { } return null; } - // For each method, copy it into the new class making appropriate adjustments + /** * Visit a method in the class and build an appropriate representation for it to include in the extracted output. */ diff --git a/springloaded/src/test/java/org/springsource/loaded/test/CrossLoaderTests.java b/springloaded/src/test/java/org/springsource/loaded/test/CrossLoaderTests.java index ec224d6..4df499f 100644 --- a/springloaded/src/test/java/org/springsource/loaded/test/CrossLoaderTests.java +++ b/springloaded/src/test/java/org/springsource/loaded/test/CrossLoaderTests.java @@ -459,4 +459,18 @@ public class CrossLoaderTests extends SpringLoadedTests { // set the superclass for a reloadabletype when the clinit runs for the subtype, for quicker lookup of exactly the type we need (getRegistryFor(clazz.getClassLoader()) // use those sparse arrays and ID numbers more so that type lookups can be quicker. Once we truly discover the right super type reference from a subtype, fill it in in the array // optimize for the case where there will only be one type around of a given name *usually* + + @Test + public void github34() throws Exception { + ReloadableType rtypeA = subLoader.loadAsReloadableType("issue34.Implementation3"); + result = runUnguarded(rtypeA.getClazz(), "run"); + assertEquals("Hello World!", result.stdout); + +// ReloadableType rtypeB = subLoader.loadAsReloadableType("subpkg.Bottom"); +// result = runUnguarded(rtypeB.getClazz(), "m"); +// assertEquals("Bottom.m() running", result.stdout); +// assertNotSame(rtypeA.getTypeRegistry(), rtypeB.getTypeRegistry()); + } + + } \ No newline at end of file diff --git a/springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java b/springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java index fc1bdad..257a1ab 100644 --- a/springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java +++ b/springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java @@ -18,6 +18,7 @@ package org.springsource.loaded.test; import static org.junit.Assert.fail; import org.junit.AfterClass; +import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; import org.springsource.loaded.test.ReloadingJVM.JVMOutput; @@ -54,6 +55,25 @@ public class SpringLoadedTestsInSeparateJVM extends SpringLoadedTests { JVMOutput output = jvm.run("jvmtwo.Runner"); assertStdout("jvmtwo.Runner.run() running", output); } + + @Test + public void githubIssue34() throws Exception { + jvm.copyToTestdataDirectory("issue34.Interface1"); + jvm.copyToTestdataDirectory("issue34.Interface2"); + jvm.copyToTestdataDirectory("issue34.Implementation1"); + jvm.copyToTestdataDirectory("issue34.Implementation2"); + jvm.copyToTestdataDirectory("issue34.Implementation3"); + JVMOutput output = jvm.run("issue34.Implementation3"); + assertStdout("Hello World!\n", output); + } + + @Test + public void githubIssue34_2() throws Exception { + jvm.copyToTestdataDirectory("issue34.InnerEnum$sorters"); + jvm.copyToTestdataDirectory("issue34.InnerEnum$sorters$1"); + JVMOutput output = jvm.run("issue34.InnerEnum"); + assertStdout("Hello World!\n", output); + } @Test public void testCreatingAndInvokingMethodsOnInstance() throws Exception { diff --git a/springloaded/src/test/java/org/springsource/loaded/test/infra/SubLoader.java b/springloaded/src/test/java/org/springsource/loaded/test/infra/SubLoader.java index 8af4e76..d2bd283 100644 --- a/springloaded/src/test/java/org/springsource/loaded/test/infra/SubLoader.java +++ b/springloaded/src/test/java/org/springsource/loaded/test/infra/SubLoader.java @@ -40,6 +40,8 @@ import org.springsource.loaded.test.TestUtils; */ public class SubLoader extends ClassLoader { + public static final boolean DEBUG_LOADING = false; + // @formatter:off static String[] folders = new String[] { TestUtils.getPathToClasses("../testdata-subloader") @@ -60,6 +62,9 @@ public class SubLoader extends ClassLoader { } public ReloadableType loadAsReloadableType(String typename) throws ClassNotFoundException { + if (DEBUG_LOADING) { + System.out.println("SubLoader: loading "+typename); + } if (tr == null) { tr = TypeRegistry.getTypeRegistryFor(this); } diff --git a/springloaded/src/test/java/org/springsource/loaded/test/infra/SuperLoader.java b/springloaded/src/test/java/org/springsource/loaded/test/infra/SuperLoader.java index e0a27d2..b2af228 100644 --- a/springloaded/src/test/java/org/springsource/loaded/test/infra/SuperLoader.java +++ b/springloaded/src/test/java/org/springsource/loaded/test/infra/SuperLoader.java @@ -73,7 +73,9 @@ public class SuperLoader extends ClassLoader { } public Class findClass(String name) throws ClassNotFoundException { - // System.out.println(">> SuperLoader.findClass(" + name + ")"); + if (SubLoader.DEBUG_LOADING) { + System.out.println("> SuperLoader: findClass(" + name + ")"); + } Class c = null; // Look in the filesystem first try { @@ -87,6 +89,9 @@ public class SuperLoader extends ClassLoader { ReloadableType rtype = tr.addType(name, data); data = rtype.bytesLoaded; } + if (SubLoader.DEBUG_LOADING) { + System.out.println(" SuperLoader: found in folder: '"+folders[i]+"'"); + } c = defineClass(name, data, 0, data.length); break; } @@ -105,6 +110,7 @@ public class SuperLoader extends ClassLoader { if (zipentry != null) { byte[] data = Utils.loadBytesFromStream(zipfile.getInputStream(zipentry)); TypeRegistry tr = TypeRegistry.getTypeRegistryFor(this); +// data = new SpringLoadedPreProcessor().preProcess(this, slashedClassName, null, data); if (tr != null) { // Give the plugins a chance to rewrite stuff too @@ -120,7 +126,9 @@ public class SuperLoader extends ClassLoader { //System.out.println("Transforming " + name); data = tr.methodCallRewrite(data); } - + if (SubLoader.DEBUG_LOADING) { + System.out.println(" SuperLoader: found in zip: '"+jars[i]+"'"); + } c = defineClass(name, data, 0, data.length); break; } diff --git a/testdata-subloader/src/main/java/issue34/Implementation2.java b/testdata-subloader/src/main/java/issue34/Implementation2.java new file mode 100644 index 0000000..e720da2 --- /dev/null +++ b/testdata-subloader/src/main/java/issue34/Implementation2.java @@ -0,0 +1,4 @@ +package issue34; +public abstract class Implementation2 extends grails.Implementation1 implements Interface2 { + +} \ No newline at end of file diff --git a/testdata-subloader/src/main/java/issue34/Implementation3.java b/testdata-subloader/src/main/java/issue34/Implementation3.java new file mode 100644 index 0000000..e34cb53 --- /dev/null +++ b/testdata-subloader/src/main/java/issue34/Implementation3.java @@ -0,0 +1,12 @@ +package issue34; +public class Implementation3 extends Implementation2 { + + public static void main(String [] args) { + System.out.println("Hello World!"); + } + + public static void run() { + Implementation3.main(null); + } + +} \ No newline at end of file diff --git a/testdata-subloader/src/main/java/issue34/Interface2.java b/testdata-subloader/src/main/java/issue34/Interface2.java new file mode 100644 index 0000000..7db2d65 --- /dev/null +++ b/testdata-subloader/src/main/java/issue34/Interface2.java @@ -0,0 +1,4 @@ +package issue34; +public interface Interface2 extends grails.Interface1 { + +} \ No newline at end of file diff --git a/testdata-superloader/src/main/java/grails/Implementation1.java b/testdata-superloader/src/main/java/grails/Implementation1.java new file mode 100644 index 0000000..0906e8e --- /dev/null +++ b/testdata-superloader/src/main/java/grails/Implementation1.java @@ -0,0 +1,10 @@ +package grails; + +public abstract class Implementation1 implements Interface1 { + + @Override + public final T process1(T type) { + return type; + } + +} \ No newline at end of file diff --git a/testdata-superloader/src/main/java/grails/Interface1.java b/testdata-superloader/src/main/java/grails/Interface1.java new file mode 100644 index 0000000..5c29085 --- /dev/null +++ b/testdata-superloader/src/main/java/grails/Interface1.java @@ -0,0 +1,6 @@ +package grails; +public interface Interface1 { + + public T process1(T type); + +} \ No newline at end of file diff --git a/testdata/src/main/java/issue34/Implementation1.java b/testdata/src/main/java/issue34/Implementation1.java new file mode 100644 index 0000000..38a0126 --- /dev/null +++ b/testdata/src/main/java/issue34/Implementation1.java @@ -0,0 +1,10 @@ +package issue34; + +public abstract class Implementation1 implements Interface1 { + + @Override + public final T process1(T type) { + return type; + } + +} \ No newline at end of file diff --git a/testdata/src/main/java/issue34/Implementation2.java b/testdata/src/main/java/issue34/Implementation2.java new file mode 100644 index 0000000..3681481 --- /dev/null +++ b/testdata/src/main/java/issue34/Implementation2.java @@ -0,0 +1,4 @@ +package issue34; +public abstract class Implementation2 extends Implementation1 implements Interface2 { + +} \ No newline at end of file diff --git a/testdata/src/main/java/issue34/Implementation3.java b/testdata/src/main/java/issue34/Implementation3.java new file mode 100644 index 0000000..e34cb53 --- /dev/null +++ b/testdata/src/main/java/issue34/Implementation3.java @@ -0,0 +1,12 @@ +package issue34; +public class Implementation3 extends Implementation2 { + + public static void main(String [] args) { + System.out.println("Hello World!"); + } + + public static void run() { + Implementation3.main(null); + } + +} \ No newline at end of file diff --git a/testdata/src/main/java/issue34/InnerEnum.java b/testdata/src/main/java/issue34/InnerEnum.java new file mode 100644 index 0000000..c745261 --- /dev/null +++ b/testdata/src/main/java/issue34/InnerEnum.java @@ -0,0 +1,29 @@ +package issue34; + +import java.util.Comparator; +import java.util.Map; +import java.util.TreeMap; + +public class InnerEnum { + + public static void run() { + InnerEnum.main(null); + } + + @SuppressWarnings("unused") + public static void main(String[] args) { + System.out.println("Hello World!"); + Map map = new TreeMap(sorters.string); + } + + private static enum sorters implements Comparator { + string { + private static final long serialVersionUID = 1L; + + @Override + public int compare(String o1, String o2) { + return o1.compareTo(o2); + } + } + } +} \ No newline at end of file diff --git a/testdata/src/main/java/issue34/Interface1.java b/testdata/src/main/java/issue34/Interface1.java new file mode 100644 index 0000000..995605b --- /dev/null +++ b/testdata/src/main/java/issue34/Interface1.java @@ -0,0 +1,6 @@ +package issue34; +public interface Interface1 { + + public T process1(T type); + +} \ No newline at end of file diff --git a/testdata/src/main/java/issue34/Interface2.java b/testdata/src/main/java/issue34/Interface2.java new file mode 100644 index 0000000..1a0cae4 --- /dev/null +++ b/testdata/src/main/java/issue34/Interface2.java @@ -0,0 +1,4 @@ +package issue34; +public interface Interface2 extends Interface1 { + +} \ No newline at end of file