diff --git a/src/main/groovy/util/GroovyScriptEngine.java b/src/main/groovy/util/GroovyScriptEngine.java index b22ba7f..97c7239 100644 --- a/src/main/groovy/util/GroovyScriptEngine.java +++ b/src/main/groovy/util/GroovyScriptEngine.java @@ -92,8 +92,10 @@ public class GroovyScriptEngine implements ResourceConnector { private URL[] roots; private ResourceConnector rc; private final ClassLoader parentLoader; - private final GroovyClassLoader groovyLoader; + private final ScriptClassLoader groovyLoader; private final Map scriptCache = new ConcurrentHashMap(); + // GROOVY-4975: We need to maintain a list a classes that have a GSE script as their origin, mapped to the sources. + private final Map> scriptPrimaryNodes = new ConcurrentHashMap>(); private CompilerConfiguration config = new CompilerConfiguration(CompilerConfiguration.DEFAULT); //TODO: more finals? @@ -165,7 +167,15 @@ public class GroovyScriptEngine implements ResourceConnector { // GROOVY-4013: If it is an inner class, tracking its dependencies doesn't really // serve any purpose and also interferes with the caching done to track dependencies if (classNode instanceof InnerClassNode) return; - DependencyTracker dt = new DependencyTracker(source, cache); + DependencyTracker dt = new DependencyTracker(source, cache) { + // GROOVY-4975: If a node is one of our scripts we've seen earlier, we should treat it + // as a primary node even though from a CompilationUnit/ClassLoader perspective + // it isn't. + @Override + protected boolean shouldAddToCache(ClassNode node) { + return scriptPrimaryNodes.containsKey(node.getName()) || super.shouldAddToCache(node); + } + }; dt.visitClass(classNode); } }, Phases.CLASS_GENERATION); @@ -211,6 +221,8 @@ public class GroovyScriptEngine implements ResourceConnector { Set value = convertToPaths(entry.getValue()); ScriptCacheEntry cacheEntry = new ScriptCacheEntry(clazz, now, value); scriptCache.put(entryName, cacheEntry); + // GROOVY-4975. Remember the class as resulting from a script. + scriptPrimaryNodes.put(clazz.getName(), value); } cache.clear(); localCu.set(null); @@ -221,7 +233,7 @@ public class GroovyScriptEngine implements ResourceConnector { ThreadLocal localCu = getLocalCompilationUnit(); String name = clazz.getName(); ClassNode classNode = localCu.get().getClassNode(name); - return classNode.getModule().getContext().getName(); + return classNode == null ? null : classNode.getModule().getContext().getName(); } private Set convertToPaths(Set orig) { @@ -229,10 +241,23 @@ public class GroovyScriptEngine implements ResourceConnector { for (String className : orig) { Class clazz = getClassCacheEntry(className); if (clazz == null) continue; - ret.add(getPath(clazz)); + String pathFromCu = getPath(clazz); + // GROOVY-4975. Paths may come from our current CU, of from previous compilation. + if(pathFromCu != null) { + ret.add(pathFromCu); + } else { + ret.addAll(scriptPrimaryNodes.get(clazz.getName())); + } } return ret; } + + /* GROOVY-4975. We need this to remove classes that have become outdated because a dependency was + * updated, override to enlarge access from protected. */ + @Override + public void removeClassCacheEntry(String name) { + super.classCache.remove(name); + } } /** @@ -259,13 +284,15 @@ public class GroovyScriptEngine implements ResourceConnector { } /** - * Initialize a new GroovyClassLoader with a default or + * Initialize a new ScriptClassLoader with a default or * constructor-supplied parentClassLoader. + * GROOVY-4975: We use an explicit ScriptClassLoader rather + * than a GroovyClassLoader to be able to call removeClassCacheEntry. * * @return the parent classloader used to load scripts */ - private GroovyClassLoader initGroovyLoader() { - return (GroovyClassLoader) AccessController.doPrivileged(new PrivilegedAction() { + private ScriptClassLoader initGroovyLoader() { + return (ScriptClassLoader) AccessController.doPrivileged(new PrivilegedAction() { public Object run() { if (parentLoader instanceof GroovyClassLoader) { return new ScriptClassLoader((GroovyClassLoader) parentLoader); @@ -557,6 +584,15 @@ public class GroovyScriptEngine implements ResourceConnector { if (depEntry.lastModified < lastMod) { ScriptCacheEntry newEntry = new ScriptCacheEntry(depEntry.scriptClass, lastMod, depEntry.dependencies); scriptCache.put(scriptName, newEntry); + /* GROOVY-4975 Removing entries from scriptcache and classcache that depended on the entry we just + * updated; these should be recompiled. */ + for(Map.Entry currentEntry : scriptCache.entrySet()) { + if(!currentEntry.getKey().equals(scriptName) && + currentEntry.getValue().dependencies.contains(scriptName)) { + scriptCache.remove(currentEntry.getKey()); + groovyLoader.removeClassCacheEntry(currentEntry.getValue().scriptClass.getName()); + } + } return true; } } diff --git a/src/main/org/codehaus/groovy/tools/gse/DependencyTracker.java b/src/main/org/codehaus/groovy/tools/gse/DependencyTracker.java index 8a1ed46..03310c7 100644 --- a/src/main/org/codehaus/groovy/tools/gse/DependencyTracker.java +++ b/src/main/org/codehaus/groovy/tools/gse/DependencyTracker.java @@ -43,8 +43,14 @@ public class DependencyTracker extends ClassCodeVisitorSupport { } private void addToCache(ClassNode node){ - if (!node.isPrimaryClassNode()) return; - current.add(node.getName()); + if (shouldAddToCache(node)) { + current.add(node.getName()); + } + } + + /* Protected to allow the GroovyScriptEngine to override this. */ + protected boolean shouldAddToCache(ClassNode node) { + return node.isPrimaryClassNode(); } private void addToCache(ClassNode[] nodes){ diff --git a/src/test/groovy/util/GroovyScriptEngineReloadingDependencyTest.groovy b/src/test/groovy/util/GroovyScriptEngineReloadingDependencyTest.groovy index e69de29..50a336a 100644 --- a/src/test/groovy/util/GroovyScriptEngineReloadingDependencyTest.groovy +++ b/src/test/groovy/util/GroovyScriptEngineReloadingDependencyTest.groovy @@ -0,0 +1,149 @@ +package groovy.util + +/** + * This testcase runs a couple of tests related to correctly reloading/recompiling + * dependent classes in the script engine, in case of multiple incoming dependencies. + * See GROOVY-4975. + * + * This class uses the MapFileSystem from GroovyScriptEngineReloadingTest. + * + * @author Frans van Buul + */ +class GroovyScriptEngineReloadingDependencyTest extends GroovyTestCase { + + GroovyScriptEngine gse; + + void setUp() { + MapFileSystem.instance.registerMapFileSystem() + gse = new GroovyScriptEngine([MapUrlConnection.URL_SCHEME] as String[]) + gse.config.minimumRecompilationInterval = 100 + } + + /** + * Writes a starting set of script sources to the map file system. These are + * common to all our tests. + */ + void writeSources() { + /* This is the bean that we will modify in our tests. This should pose + * no problems for other classes that depend on it. */ + MapFileSystem.instance.modFile('MyBean.groovy', + '''class MyBean { int a = Constants.a }''', + new Date().time) + + /* Introducing a circular dependency, just to verify that this doesn't create + * a problem. */ + MapFileSystem.instance.modFile('Constants.groovy', + '''class Constants { static int a = 1; static MyBean myBean = new MyBean() }''', + new Date().time) + + /* This class casts an object to a MyBean. Reloading problems will manifest + * themselves as a class cast exception. */ + MapFileSystem.instance.modFile('Extractor.groovy', + '''class Extractor { def getBeanA(def bean) { MyBean.class.cast(bean).a } }''', + new Date().time) + + /* These classes instantiate MyBean and return the value of the a property. + * Reloading problems will cause unexpected data to be returned. */ + MapFileSystem.instance.modFile('Consumer1.groovy', + '''class Consumer1 { def getBeanA() { (new MyBean()).a } }''', + new Date().time) + MapFileSystem.instance.modFile('Consumer2.groovy', + '''class Consumer2 { def getBeanA() { (new MyBean()).a } }''', + new Date().time) + + /* Sleeping to avoid any timing issue. */ + sleep 1000 + } + + /** + * Modifies MyBean to have a = 2 rather than a = 1. + */ + void modifyMyBean() { + MapFileSystem.instance.modFile('MyBean.groovy', + '''class MyBean { int a = Constants.a + 1 }''', + new Date().time) + + /* Sleeping to avoid any timing issue. */ + sleep 1000 + } + + /** + * Modifies Constants to have a = 3 rather than a = 1. + */ + void modifyConstants() { + MapFileSystem.instance.modFile('Constants.groovy', + '''class Constants { static int a = 3; static MyBean myBean = new MyBean() }''', + new Date().time) + + /* Sleeping to avoid any timing issue. */ + sleep 1000 + } + + /** + * Tests whether Extractor is correctly recompiled after MyBean is changed + * and then instantiated/recompiled before Extractor is. + * + * Implementation-wise, this specifically tests that a recorded dependency + * from Extractor and MyBean forces recompilation when needed. + */ + void testReloadCast() { + writeSources() + + def extractor = gse.loadScriptByName('Extractor.groovy').newInstance() + def myBean = gse.loadScriptByName('MyBean.groovy').newInstance() + assert extractor.getBeanA(myBean) == 1 + + modifyMyBean() + + myBean = gse.loadScriptByName('MyBean.groovy').newInstance() + assert gse.loadScriptByName('Extractor.groovy').newInstance().getBeanA(myBean) == 2 + } + + /** + * Tests whether Consumer2 is correctly recompiled after MyBean is changed + * and Consumer1/MyBean are recompiled before Consumer2 is. + * + * Implementation-wise, this specifically tests that the dependency from + * Consumer2 to MyBean is correctly recorded and taken into account, even though MyBean + * is already compiled when Consumer2 gets compiled. + */ + void testReloadData() { + writeSources() + + assert gse.loadScriptByName('Consumer1.groovy').newInstance().beanA == 1 + assert gse.loadScriptByName('Consumer2.groovy').newInstance().beanA == 1 + + modifyMyBean() + + assert gse.loadScriptByName('Consumer1.groovy').newInstance().beanA == 2 + assert gse.loadScriptByName('Consumer2.groovy').newInstance().beanA == 2 + } + + /** + * Here, were modify Constants rather than MyBean to check that everything works + * if a dependency is indirect. + * + * Implementation-wise, this specifically tests + * 1. that the transitive hull of the 'bare' dependency graph rather than the + * 'bare' dependency graph itself is used for making reloading decisions. + * 2. that changes to class-fields are taken into account. If the script engine + * correctly handles the ScriptCache, but not the caching in the ScriptClassLoader, + * this test fails while the previous tests may have succeeded. + */ + void testIndirect() { + writeSources() + + assert gse.loadScriptByName('Consumer1.groovy').newInstance().beanA == 1 + assert gse.loadScriptByName('Consumer2.groovy').newInstance().beanA == 1 + def extractor = gse.loadScriptByName('Extractor.groovy').newInstance() + def myBean = gse.loadScriptByName('MyBean.groovy').newInstance() + assert extractor.getBeanA(myBean) == 1 + + modifyConstants() + + assert gse.loadScriptByName('Consumer1.groovy').newInstance().beanA == 3 + assert gse.loadScriptByName('Consumer2.groovy').newInstance().beanA == 3 + myBean = gse.loadScriptByName('MyBean.groovy').newInstance() + assert gse.loadScriptByName('Extractor.groovy').newInstance().getBeanA(myBean) == 3 + } +}