From 3fc5dd056d46a87313516727b39422fed57be0e1 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Mon, 20 Jul 2015 21:57:46 -0700 Subject: [PATCH] Reworking split package related changes. Instead of considering things from jars to be reloadable if they are in the same package as subtypes that are also reloadable it is better to just make the test (isTopmostReloadable()) smarter. This test determines which reloadable type is the highest in the hierarchy and gets extra state added to it. With a 'dumb test' it was assuming further parents were reloadable because they were in the same package as the subtype. Now it does a more costly, more precise check. The original solution had problems because if you made some classes from a jar reloadable, that confused handled of other subclasses of those types in other jars (that did not share the same package name so would not have been made reloadable). This area of the code to do with a bit of a smack with a refactoring hammer to streamline it. --- .../loaded/ConstructorCopier.java | 22 +++--- .../org/springsource/loaded/TypeRegistry.java | 42 +++++----- .../org/springsource/loaded/TypeRewriter.java | 74 +++++++++++++++--- .../agent/SpringLoadedPreProcessor.java | 22 +++--- .../test/SpringLoadedTestsInSeparateJVM.java | 54 ++++++++++++- testdata/jars/grailsplugins/A.class | Bin 258 -> 0 bytes testdata/jars/grailsplugins/build.sh | 10 ++- .../jars/grailsplugins/grails/plugins/A.class | Bin 485 -> 0 bytes testdata/jars/grailsplugins/grailsplugins.jar | Bin 486 -> 944 bytes testdata/src/main/java/grails/plugins2/C.java | 10 +++ 10 files changed, 178 insertions(+), 56 deletions(-) delete mode 100644 testdata/jars/grailsplugins/A.class delete mode 100644 testdata/jars/grailsplugins/grails/plugins/A.class create mode 100644 testdata/src/main/java/grails/plugins2/C.java 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 d822b42ba1a86818d4455d5e3baf795765e1f328..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 258 zcmZ8bONzok6s#BX8Dsq54Q8h01qX;7FV;W zdR4`v>i)Ri0L(B5;1C9BDYdD@$uz0XE3pjVvYW|2DU8fh@e^k%sR{m<&UL*Yv_|6{ z!Ce)56{3Z91P?x;ztOq+ZjP}kw=y=0Fx(W0G&@;ptKX(uA9Q82|GjfU_**n(qSo4e w`^%{vflqKH4+b;YD#n17cxk|!T<~#b!$HVsWAH z?*wvmqK}~HP!&*Sl#9o2C_iViN72flh9!#r>;dIvsu))0&8Ha*a9>cVHD0l?0C*%> QOqr%EAnqKpM`{lH-?xxnxc~qF diff --git a/testdata/jars/grailsplugins/grailsplugins.jar b/testdata/jars/grailsplugins/grailsplugins.jar index 693db966692fc34a4c129067aeed9b5552cc5104..c6f9eb4a466fefe1deb1b9f4150ceae27f4dfd42 100644 GIT binary patch delta 522 zcmaFHyn&rJz?+#xgn@&DgTb`p^G4oU#(E^a8v_y$2eQ+P5;Jp(^$T)J(=+pmjr5)M zl5-M^i*p0Ky$?GG{M**Qr1RpwHTi4rMe)bJIO^;=p<%g#)6(y`y@jfGbt^gk>DX#i zd|>~v_=u;@)I?zJKQ3@#nX#8|-eWaq)ar5UMxo`KjZ>yz-Gqs)=tDt delta 61 zcmdnM{*0M7z?+#xgn@&DgF!Ru-A3M8MqWlH5eB%(WEo~_5mq(^AYcSS4