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.

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'
 						}

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);
 						}

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", "<init>", "()V");

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);

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");

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
 

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");
+	}
+}