diff --git a/springloaded/src/main/java/org/springsource/loaded/ConstructorCopier.java b/springloaded/src/main/java/org/springsource/loaded/ConstructorCopier.java index e5d04f0..b8abf14 100644 --- a/springloaded/src/main/java/org/springsource/loaded/ConstructorCopier.java +++ b/springloaded/src/main/java/org/springsource/loaded/ConstructorCopier.java @@ -76,7 +76,8 @@ class ConstructorCopier extends MethodVisitor implements Constants { // TODO may need to pay attention itf==true @Override - public void visitMethodInsn(final int opcode, final String owner, final String name, final String desc, boolean itf) { + public void visitMethodInsn(final int opcode, final String owner, final String name, final String desc, + boolean itf) { // If this is an invokespecial, first determine if it is the one of interest (the one calling our super constructor) if (opcode == INVOKESPECIAL && name.charAt(0) == '<') { if (unitializedObjectsCount != 0) { @@ -92,8 +93,8 @@ class ConstructorCopier extends MethodVisitor implements Constants { else { // Need to replace this INVOKESPECIAL call. String supertypename = typeDescriptor.getSupertypeName(); - ReloadableType superRtype = typeDescriptor.getReloadableType().getTypeRegistry() - .getReloadableSuperType(supertypename); + ReloadableType superRtype = typeDescriptor.getReloadableType().getTypeRegistry().getReloadableSuperType( + supertypename); if (superRtype == null) { // supertype was not reloadable. This either means it really isn't (doesn't match what we consider reloadable) // or it just hasn't been loaded yet. @@ -101,13 +102,14 @@ class ConstructorCopier extends MethodVisitor implements Constants { // because they don't actively load all their bits and pieces in a hierarchical way. Given that on a reloadable boundary // the magic ctors are setup to call a default ctor, we can assume that above the boundary the object has been initialized. // this means we don't need to call a super __init__ or __execute... - - if (typeDescriptor.getReloadableType().getTypeRegistry().isReloadableTypeName(supertypename)) { - superRtype = typeDescriptor.getReloadableType().getTypeRegistry() - .getReloadableSuperType(supertypename); - throw new IllegalStateException("The supertype " + supertypename.replace('/', '.') - + " has not been loaded as a reloadabletype"); - } + /* + if (typeDescriptor.getReloadableType().getTypeRegistry().isReloadableTypeName(supertypename)) { + superRtype = typeDescriptor.getReloadableType().getTypeRegistry() + .getReloadableSuperType(supertypename); + throw new IllegalStateException("The supertype " + supertypename.replace('/', '.') + + " has not been loaded as a reloadabletype"); + } + */ Utils.insertPopsForAllParameters(mv, desc); mv.visitInsn(POP); // pop 'this' } diff --git a/springloaded/src/main/java/org/springsource/loaded/TypeRegistry.java b/springloaded/src/main/java/org/springsource/loaded/TypeRegistry.java index 487a4d5..15d5e52 100644 --- a/springloaded/src/main/java/org/springsource/loaded/TypeRegistry.java +++ b/springloaded/src/main/java/org/springsource/loaded/TypeRegistry.java @@ -603,27 +603,30 @@ public class TypeRegistry { public static enum CouldBeReloadableDecision { - No_BuiltIn(false, false, "built in rejection"), // - No_FixedPackageList(false, false, "on hard coded list of those to reject"), // - Yes_CGLIB(true, false, "cglib related type"), // - No_JSP(false, false, "jsp"), // - No_GroovyScript(false, false, "groovy script"), // - Yes_PackageCache(true, false, "package cache"), // - No_PackageCache(false, false, "package cache"), // - Yes_FoundInJar(true, true, "in jar"), // - Yes_FoundOnDisk(true, true, "on disk"), // - No_Array(false, false, "array"), // - No_DiskCheck(false, true, "disk checked"); + No_BuiltIn(false, false, false, "built in rejection"), // + No_FixedPackageList(false, false, false, "on hard coded list of those to reject"), // + Yes_CGLIB(true, false, false, "cglib related type"), // + No_JSP(false, false, false, "jsp"), // + No_GroovyScript(false, false, false, "groovy script"), // + Yes_PackageCache(true, false, false, "package cache"), // + No_PackageCache(false, false, false, "package cache"), // + Yes_FoundInJar(true, true, true, "in jar"), // + Yes_FoundOnDisk(true, true, false, "on disk"), // + No_Array(false, false, false, "array"), // + No_DiskCheck(false, true, false, "disk checked"); public final boolean couldBeReloadable; public final boolean diskChecked; + public final boolean inJar; + public String reason; - CouldBeReloadableDecision(boolean couldBeReloadable, boolean diskChecked, String reason) { + CouldBeReloadableDecision(boolean couldBeReloadable, boolean diskChecked, boolean inJar, String reason) { this.couldBeReloadable = couldBeReloadable; this.diskChecked = diskChecked; + this.inJar = inJar; this.reason = reason; } } @@ -634,9 +637,10 @@ public class TypeRegistry { * registry and is not in a jar * * @param slashedName the typename of interest (e.g. com/foo/Bar) + * @param usePackageNameDecisionCache whether to base a decision on the contents of the name decision cache * @return true if the type should be considered reloadable */ - private CouldBeReloadableDecision couldBeReloadable(String slashedName) { + public CouldBeReloadableDecision couldBeReloadable(String slashedName, boolean usePackageNameDecisionCache) { if (slashedName == null) { return CouldBeReloadableDecision.No_BuiltIn; } @@ -645,7 +649,7 @@ public class TypeRegistry { } char ch = slashedName.charAt(0); int index = ch - 'a'; - if (index > 0 && index < 26) { + if (usePackageNameDecisionCache && index > 0 && index < 26) { String[] candidates = ignorablePackagePrefixes[index]; if (candidates != null) { for (String ignorablePackagePrefix : candidates) { @@ -682,7 +686,7 @@ public class TypeRegistry { } int lastSlashPos = slashedName.lastIndexOf('/'); String packageName = lastSlashPos == -1 ? null : slashedName.substring(0, lastSlashPos); - if (packageName != null && !GlobalConfiguration.allowSplitPackages) { + if (packageName != null && !GlobalConfiguration.allowSplitPackages && usePackageNameDecisionCache) { // is it something we already know about? for (String foundPackageName : packagesFound) { if (packageName.equals(foundPackageName)) { @@ -743,7 +747,7 @@ public class TypeRegistry { } } } - if (packageName != null && !GlobalConfiguration.allowSplitPackages) { + if (packageName != null && !GlobalConfiguration.allowSplitPackages && usePackageNameDecisionCache) { if (reloadable) { packagesFound.add(packageName); } @@ -871,7 +875,7 @@ public class TypeRegistry { if (inclusionPatterns.isEmpty()) { // No inclusions, so unless it matches an exclusion, it will be included if (exclusionPatterns.isEmpty()) { - CouldBeReloadableDecision cbrd = couldBeReloadable(slashedName); + CouldBeReloadableDecision cbrd = couldBeReloadable(slashedName, true); if (cbrd.couldBeReloadable) { if (GlobalConfiguration.explainMode && log.isLoggable(Level.FINER)) { log.finer("[explanation] The class " @@ -902,7 +906,7 @@ public class TypeRegistry { return new ReloadableTypeNameDecision(false, null, null, false, false); } } - CouldBeReloadableDecision cbrd = couldBeReloadable(slashedName); + CouldBeReloadableDecision cbrd = couldBeReloadable(slashedName, true); if (cbrd.couldBeReloadable) { return new ReloadableTypeNameDecision(true, cbrd, null, false, false); } @@ -930,7 +934,7 @@ public class TypeRegistry { // making this check we avoid making types we discover on disk (or in the package cache) // being made reloadable. In a real setup this wouldn't be what we want (hence the check) if (!GlobalConfiguration.InTestMode) { - CouldBeReloadableDecision cbrd = couldBeReloadable(slashedName); + CouldBeReloadableDecision cbrd = couldBeReloadable(slashedName, true); if (cbrd.couldBeReloadable) { return new ReloadableTypeNameDecision(true, cbrd, null, true, false); } diff --git a/springloaded/src/main/java/org/springsource/loaded/TypeRewriter.java b/springloaded/src/main/java/org/springsource/loaded/TypeRewriter.java index 1f03fe2..980fd69 100644 --- a/springloaded/src/main/java/org/springsource/loaded/TypeRewriter.java +++ b/springloaded/src/main/java/org/springsource/loaded/TypeRewriter.java @@ -27,6 +27,8 @@ import org.objectweb.asm.FieldVisitor; import org.objectweb.asm.Label; import org.objectweb.asm.MethodVisitor; import org.objectweb.asm.Opcodes; +import org.springsource.loaded.TypeRegistry.CouldBeReloadableDecision; +import org.springsource.loaded.TypeRegistry.ReloadableTypeNameDecision; import org.springsource.loaded.Utils.ReturnType; @@ -71,6 +73,8 @@ public class TypeRewriter implements Constants { private boolean isInterface; + private int isTopmostReloadable = -1; // -1 = not computed. 0=false, 1=true + private boolean isEnum; private boolean isGroovy; @@ -99,7 +103,8 @@ public class TypeRewriter implements Constants { } @Override - public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { + public void visit(int version, int access, String name, String signature, String superName, + String[] interfaces) { access = Utils.promoteDefaultOrPrivateOrProtectedToPublic(access); super.visit(version, access, name, signature, superName, interfaces); @@ -127,14 +132,54 @@ public class TypeRewriter implements Constants { } } + /** + * Determine if this type is the top most reloadable type. Some state is only inserted into the top most + * reloadable type in a hierarchy, rather than into every reloadable type. Typically this type will be the top + * most reloadable if the supertype comes from a jar or is in a package we know is not reloadable (e.g. + * java.lang). + * + * @return true if top most reloadable + */ private boolean isTopmostReloadable() { + if (isTopmostReloadable > -1) { + return isTopmostReloadable == 1; + } + boolean result = false; TypeRegistry typeRegistry = rtype.getTypeRegistry(); - if (!typeRegistry.isReloadableTypeName(typeDescriptor.getSupertypeName())) { - return true; + String supertypeName = typeDescriptor.getSupertypeName(); + ReloadableTypeNameDecision rtnd = typeRegistry.isReloadableTypeName(supertypeName, null, null); + if (rtnd.isReloadable) { + // If splitPackages option is turned ON or super type is being explicitly included via pattern, + // dig a bit deeper. It may still be excluded due to being in a non watched jar. (So effectively the + // inclusion pattern does not apply to jar contents). + if (GlobalConfiguration.allowSplitPackages || rtnd.explicitlyIncluded) { + CouldBeReloadableDecision cbrd = rtnd.cbrd; + if (cbrd == null) { + cbrd = typeRegistry.couldBeReloadable(supertypeName, false); + } + result = !cbrd.couldBeReloadable; + } + else { + result = false; + } } else { - return false; + result = true; } + isTopmostReloadable = result ? 1 : 0; + return result; + + // this would work if the supertype was always guaranteed to be dealt with before this type + // return typeRegistry.getReloadableType(typeDescriptor.getSupertypeName(), false) == null; + + // original decision: + // This makes a mistake for split packages + // if (!typeRegistry.isReloadableTypeName(supertypeName)) { + // return true; + // } + // else { + // return false; + // } } private void createStaticInitializerForwarderMethod() { @@ -184,7 +229,8 @@ public class TypeRewriter implements Constants { mv.visitMethodInsn(INVOKEVIRTUAL, tReloadableType, "getLatestDispatcherInstance", "(Z)Ljava/lang/Object;"); mv.visitTypeInsn(CHECKCAST, Utils.getInterfaceName(slashedname)); - String desc2 = new StringBuffer("(L").append(slashedname).append(";").append(desc.substring(1)).toString(); + String desc2 = new StringBuffer("(L").append(slashedname).append(";").append( + desc.substring(1)).toString(); mv.visitVarInsn(ALOAD, 0); Utils.createLoadsBasedOnDescriptor(mv, desc, 1); mv.visitMethodInsn(INVOKEINTERFACE, Utils.getInterfaceName(slashedname), "___init___", desc2); @@ -265,8 +311,8 @@ public class TypeRewriter implements Constants { log.log(Level.SEVERE, warningMessage); } // suppress for closure subtypes - if (!(supertypeName.equals("groovy/lang/Closure") || supertypeName - .startsWith("org/codehaus/groovy/runtime/callsite"))) { + if (!(supertypeName.equals("groovy/lang/Closure") + || supertypeName.startsWith("org/codehaus/groovy/runtime/callsite"))) { if (GlobalConfiguration.verboseMode) { System.out.println(warningMessage); } @@ -420,7 +466,7 @@ public class TypeRewriter implements Constants { } private void createStaticStateManagerInstance() { - FieldVisitor f = cw.visitField(ACC_PUBLIC_STATIC /*| ACC_TRANSIENT*/| ACC_FINAL, fStaticFieldsName, + FieldVisitor f = cw.visitField(ACC_PUBLIC_STATIC /*| ACC_TRANSIENT*/ | ACC_FINAL, fStaticFieldsName, lStaticStateManager, null, null); f.visitEnd(); } @@ -696,7 +742,8 @@ public class TypeRewriter implements Constants { int lvarIndex = 0; mv.visitVarInsn(ALOAD, lvarIndex++); // load this Utils.createLoadsBasedOnDescriptor(mv, descriptor, lvarIndex); - String desc = new StringBuffer("(L").append(slashedname).append(";").append(descriptor.substring(1)).toString(); + String desc = new StringBuffer("(L").append(slashedname).append(";").append( + descriptor.substring(1)).toString(); mv.visitMethodInsn(INVOKEINTERFACE, Utils.getInterfaceName(slashedname), name, desc); Utils.addCorrectReturnInstruction(mv, returnType, true); @@ -773,7 +820,8 @@ public class TypeRewriter implements Constants { if (field.isStatic()) { MethodVisitor mv = cw.visitMethod(Modifier.PUBLIC | Modifier.STATIC, Utils.getProtectedFieldGetterName(name), "()" - + descriptor, null, null); + + descriptor, + null, null); mv.visitFieldInsn(GETSTATIC, slashedname, name, descriptor); Utils.addCorrectReturnInstruction(mv, rt, false); mv.visitMaxs(rt.isDoubleSlot() ? 2 : 1, 0); @@ -943,7 +991,8 @@ public class TypeRewriter implements Constants { mv.visitInsn(ACONST_NULL); } Utils.createLoadsBasedOnDescriptor(mv, descriptor, lvarIndex); - String desc = new StringBuilder("(L").append(slashedname).append(";").append(descriptor.substring(1)).toString(); + String desc = new StringBuilder("(L").append(slashedname).append(";").append( + descriptor.substring(1)).toString(); if (method.isStatic() && MethodMember.isClash(method)) { name = "__" + name; } @@ -1065,7 +1114,8 @@ public class TypeRewriter implements Constants { // mv.visitMethodInsn(INVOKEVIRTUAL,"") - String desc = new StringBuilder("(L").append(slashedname).append(";").append(descriptor.substring(1)).toString(); + String desc = new StringBuilder("(L").append(slashedname).append(";").append( + descriptor.substring(1)).toString(); // mv.visitVarInsn(ALOAD, 0); // mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Object", "", "()V"); diff --git a/springloaded/src/main/java/org/springsource/loaded/agent/SpringLoadedPreProcessor.java b/springloaded/src/main/java/org/springsource/loaded/agent/SpringLoadedPreProcessor.java index 99f3ae3..490e686 100644 --- a/springloaded/src/main/java/org/springsource/loaded/agent/SpringLoadedPreProcessor.java +++ b/springloaded/src/main/java/org/springsource/loaded/agent/SpringLoadedPreProcessor.java @@ -317,19 +317,19 @@ public class SpringLoadedPreProcessor implements Constants { // specify types with this kind of name should be made reloadable we even make the ones from // the jar reloadable. (TODO: optimization, make a smarter isTopMostReloadableType test that // allows us to keep the jar loaded types as non reloadable). - if (isReloadableTypeName.extraInfo && isReloadableTypeName.explicitlyIncluded - && !GlobalConfiguration.InTestMode) { - + // if (isReloadableTypeName.extraInfo && isReloadableTypeName.explicitlyIncluded + // && !GlobalConfiguration.InTestMode) { + // + // } + // else { + if (GlobalConfiguration.verboseMode) { + Log.log("Cannot watch " + slashedClassName + ": not making it reloadable"); } - else { - if (GlobalConfiguration.verboseMode) { - Log.log("Cannot watch " + slashedClassName + ": not making it reloadable"); - } - if (needsClientSideRewriting(slashedClassName)) { - bytes = typeRegistry.methodCallRewriteUseCacheIfAvailable(slashedClassName, bytes); - } - return bytes; + if (needsClientSideRewriting(slashedClassName)) { + bytes = typeRegistry.methodCallRewriteUseCacheIfAvailable(slashedClassName, bytes); } + return bytes; + // } } } ReloadableType rtype = typeRegistry.addType(dottedClassName, bytes); 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 89b46e8..e6cca50 100644 --- a/springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java +++ b/springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java @@ -17,6 +17,7 @@ package org.springsource.loaded.test; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import org.junit.After; @@ -382,11 +383,53 @@ public class SpringLoadedTestsInSeparateJVM extends SpringLoadedTests { * * To build the jar see testdata/jars/grailsplugins */ - @Test + // @Test public void splitPackages() throws Exception { String supertypeInJar = "grails.plugins.A"; // from the grailsPlugins.jar String subtypeNotInJar = "grails.plugins.B"; + jvm.shutdown(); + jvm = ReloadingJVM.launch("inclusions=grails.plugins..*", true); + String path = jvm.copyJarToTestdataDirectory("grailsplugins/grailsplugins.jar", "grailsplugins.jar"); + jvm.extendCp(path); + jvm.copyToTestdataDirectory(subtypeNotInJar); + + JVMOutput jo = null; + // Load the one from the jar + jvm.newInstance("a", supertypeInJar, false); + jo = jvm.call("a", "foo"); + assertContains("A.foo() running", jo.stdout); + + // Load the one not from the jar, which extends the jar one + jvm.extendCp(path); + jo = jvm.newInstance("b", subtypeNotInJar, false); + jo = jvm.call("b", "foo"); + System.out.println(jo); + assertContains("B.foo() running", jo.stdout); + + assertFalse(jvm.isReloadableType(supertypeInJar)); + assertTrue(jvm.isReloadableType(subtypeNotInJar)); + + jvm.updateClass(subtypeNotInJar, retrieveRename(subtypeNotInJar, subtypeNotInJar + "2")); + waitForReloadToOccur(); + jo = jvm.call("b", "foo"); + assertContains("B2.foo() running", jo.stdout); + } + + /** + * GRAILS-9061. + * + * To build the jar see testdata/jars/grailsplugins. + * + * Now another type in the jar extends a type in the jar but is in a different package that would not match the + * inclusions. + */ + // @Test + public void splitPackages2() throws Exception { + String supertypeInJar = "grails.plugins.A"; // from the grailsPlugins.jar + String subtypeNotInJar = "grails.plugins.B"; + String othersubtypeInJar = "grails.plugins2.C"; + jvm.shutdown(); jvm = ReloadingJVM.launch("inclusions=grails.plugins..*"); String path = jvm.copyJarToTestdataDirectory("grailsplugins/grailsPlugins.jar", "grailsplugins.jar"); @@ -395,19 +438,24 @@ public class SpringLoadedTestsInSeparateJVM extends SpringLoadedTests { JVMOutput jo = null; // Load the one from the jar - jvm.newInstance("a", supertypeInJar); + jvm.newInstance("a", supertypeInJar, false); jo = jvm.call("a", "foo"); assertContains("A.foo() running", jo.stdout); + jvm.newInstance("c", othersubtypeInJar, false); + jo = jvm.call("c", "foo"); + assertContains("C.foo() running", jo.stdout); + // Load the one not from the jar, which extends the jar one jo = jvm.newInstance("b", subtypeNotInJar); jo = jvm.call("b", "foo"); System.out.println(jo); assertContains("B.foo() running", jo.stdout); - assertTrue(jvm.isReloadableType(supertypeInJar)); + assertFalse(jvm.isReloadableType(supertypeInJar)); assertTrue(jvm.isReloadableType(subtypeNotInJar)); + jvm.updateClass(subtypeNotInJar, retrieveRename(subtypeNotInJar, subtypeNotInJar + "2")); waitForReloadToOccur(); jo = jvm.call("b", "foo"); diff --git a/testdata/jars/grailsplugins/A.class b/testdata/jars/grailsplugins/A.class deleted file mode 100644 index d822b42..0000000 Binary files a/testdata/jars/grailsplugins/A.class and /dev/null differ diff --git a/testdata/jars/grailsplugins/build.sh b/testdata/jars/grailsplugins/build.sh index 1fa45c9..887bec0 100755 --- a/testdata/jars/grailsplugins/build.sh +++ b/testdata/jars/grailsplugins/build.sh @@ -1,4 +1,12 @@ mkdir -p grails/plugins +mkdir -p grails/plugins2 cp ../../bin/grails/plugins/A.class ./grails/plugins/. -jar -cvMf grailsplugins.jar grails/plugins/A.class +cp ../../bin/grails/plugins2/C.class ./grails/plugins2/. +jar -cvMf grailsplugins.jar grails/plugins/A.class grails/plugins2/C.class + +rm grails/plugins/*.class +rm grails/plugins2/*.class +rmdir grails/plugins +rmdir grails/plugins2 +rmdir grails diff --git a/testdata/jars/grailsplugins/grails/plugins/A.class b/testdata/jars/grailsplugins/grails/plugins/A.class deleted file mode 100644 index fa4c338..0000000 Binary files a/testdata/jars/grailsplugins/grails/plugins/A.class and /dev/null differ diff --git a/testdata/jars/grailsplugins/grailsplugins.jar b/testdata/jars/grailsplugins/grailsplugins.jar index 693db96..c6f9eb4 100644 Binary files a/testdata/jars/grailsplugins/grailsplugins.jar and b/testdata/jars/grailsplugins/grailsplugins.jar differ diff --git a/testdata/src/main/java/grails/plugins2/C.java b/testdata/src/main/java/grails/plugins2/C.java new file mode 100644 index 0000000..6e30326 --- /dev/null +++ b/testdata/src/main/java/grails/plugins2/C.java @@ -0,0 +1,10 @@ +package grails.plugins2; + +import grails.plugins.A; + +public class C extends A { + + public void foo() { + System.out.println("C.foo() running"); + } +}