Repository navigation
[#165] Compile each runScriptOnConnector script once per engine and let evicted scripts unload - #196
Conversation
…per engine, in child loaders
…es per engine and unregister evicted ones
…per-engine class cache
…pilation failure, not a GroovyBugError
maximthomas
added
bug
…gine and compile each text with its own import customizer
…as compiled against, and compile as master did without a cache
vharseko
left a comment
vharseko
left a comment
There was a problem hiding this comment.
Thanks, the description and the test-strength table make this easy to follow. I reproduced points 1–4 with a test class that uses only API present on both master and this branch. It is at the end, under "Test used for points 1–4". All of its checks pass on master (1eeff90) and fail on 37b50d1. Point 5 comes from reading the Groovy 2.4.21 sources.
1. Under @CompileStatic, a type reached only through a signature of a pinned class is not pinned (ScriptedConfiguration.java:1112)
resolvedClasses holds only the names the compiler asked the resolver for. Some types never go through findClassNode: those a text reaches only through a member signature of a script-root class, such as the return type Helper.Greeting of Helper.make(). The static type checker takes such a type from the ClassNode of the resolved Helper. At run time the JVM still loads it through the text's loader. That lookup misses resolvedClasses and lands in the engine's current class cache.
Repro: Helper.groovy has static Greeting make() and a nested static class Greeting { String text() }. The text is @CompileStatic String use(boolean b) { b ? Helper.make().text() : 'not run' }. It is compiled and cached with b = false. Then Helper.groovy is edited and an operation script recompiles it. The same text then runs with b = true:
- this branch:
java.lang.LinkageError: loader constraint violation: loader groovy.lang.GroovyClassLoader$InnerLoader @2453f95d wants to load class Helper$Greeting. A different class with the same name was previously loaded by groovy.lang.GroovyClassLoader$InnerLoader @71e5f61d. - the same text without
@CompileStatic:hello - master:
hi
This is the case the description covers with "script-root classes such as Helper and their nested classes included". testCachedScriptLinksTheScriptRootClassesItWasCompiledAgainst names Helper.Greeting in the text, so the type goes through the resolver.
2. One compilation can mix two versions of a script-root class (ScriptedConfiguration.java:1092)
Helper resolves under scriptRootLock. Helper$Greeting has no source file of its own. It falls to super.findClassNode outside the lock and reads the engine's class cache as it is at that moment. If an operation script recompiles an edited Helper between the two lookups, the text keeps Helper v1 and Helper$Greeting v2 for as long as it stays cached.
Repro: I added a script root with a URL handler that holds the lookup of Trigger.groovy. This stops the text def a = Helper.greet(); if (false) { Trigger.x() }; def b = Helper.Greeting.text(); return a + ' ' + b after it has resolved Helper. While it waits, Helper.groovy is edited and the operation script runs.
- this branch: the operation script returns
hi hiwhile the text is still compiling, and the text returnshello hi. - master: the operation script waits for the text's compilation (the engine's compiler lock), and the text returns
hello hello.
In production the window between the two lookups is short, but the mixed set lasts as long as the cache entry does. This is a different case from the first-load race listed under behaviour changes.
For both points, a name that misses resolvedClasses could be looked up through the loader of the script-root class it is nested in. Helper v1's InnerLoader returns its own Helper$Greeting from findLoadedClass. I have not tried this.
3. Changes a subclass makes to the engine's configuration in getRoots no longer reach texts (ScriptedConfiguration.java:835)
getRoots(CompilerConfiguration, GroovyClassLoader) is protected and receives the engine's configuration (line 928). Texts now compile with a fresh newCompilerConfiguration() instead. Repro: a subclass adds an ImportCustomizer with the alias RootsList and a SecureASTCustomizer with setMethodDefinitionAllowed(false) in getRoots.
- this branch:
return new RootsList()fails withunable to resolve class RootsList, anddef f() { 1 }; return f()returns1. - master:
[]andSecurityException: Method definitions are not allowed.
Operation scripts still resolve RootsList on both branches. The description says a SecureASTCustomizer "still applies to its texts" for a subclass that builds its own engine, but this hook is a second way to configure the engine. If the change is intended, it belongs under behaviour changes. If not, the text configuration has to carry what the hook added.
4. A URL a text adds to its class loader no longer reaches the engine (ScriptedConfiguration.java:1082)
A text's InnerLoader delegates addURL, addClasspath and parseClass to the loader that compiled it. That used to be the engine's loader. Now it is the text's own ScriptOnConnectorLoader. Repro: one text runs this.class.classLoader.addURL(new URL('file:/…/lib/')), and then something else loads drv.Driver from that directory.
- this branch: the URL is not in the engine's loader, and both an operation script and a second text fail with
ClassNotFoundException: drv.Driver. - master: both load it and return
drv.
This is probably rare, but it deserves a line under behaviour changes.
5. Nit: the ScriptException branch in loadScriptRootClass (ScriptedConfiguration.java:1134)
loadScriptByName declares ScriptException, so the branch has to stay. In Groovy 2.4.21, however, the method's body never throws it. If a GroovyScriptEngine subclass did throw it, the ConnectorException thrown from inside the resolver would reach CompilationUnit.applyToSourceUnits, which turns any Exception from the resolve phase into a GroovyBugError. That is the outcome testBrokenScriptRootClassFailsAsACompilationError guards against. Throwing a CompilationFailedException there would keep it a compilation error.
Test used for points 1–4 (same package as ScriptOnConnectorCacheTest; passes on master, four failures on this branch)
package org.forgerock.openicf.misc.scriptedcommon;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.fest.assertions.api.Assertions.assertThat;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.net.URLConnection;
import java.net.URLStreamHandler;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeoutException;
import org.codehaus.groovy.control.CompilationUnit;
import org.codehaus.groovy.control.CompilerConfiguration;
import org.codehaus.groovy.control.customizers.ImportCustomizer;
import org.codehaus.groovy.control.customizers.SecureASTCustomizer;
import org.forgerock.openicf.connectors.groovy.ScriptedPoolableConnector;
import org.identityconnectors.framework.common.objects.ScriptContext;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
import groovy.lang.Binding;
import groovy.lang.GroovyClassLoader;
/** Temporary checks of the review findings on PR #196; uses only API that master has too. */
public class ReviewFindings196Test {
private static volatile CountDownLatch triggerGate;
private static volatile CountDownLatch triggerWaiting;
private File scriptRoot;
private ScriptedConfiguration configuration;
private ExecutorService executor;
@BeforeMethod
public void setUp() throws Exception {
scriptRoot = Files.createTempDirectory("review-196").toFile();
configuration = withScriptRoot(new ScriptedConfiguration());
executor = Executors.newCachedThreadPool();
}
@AfterMethod
public void tearDown() {
final CountDownLatch gate = triggerGate;
triggerGate = null;
if (null != gate) {
gate.countDown();
}
executor.shutdownNow();
configuration.release();
delete(scriptRoot);
}
/** F1: a type reached only through a signature of a pinned class, under @CompileStatic. */
@Test
public void f1SignatureOnlyNestedTypeCompileStatic() throws Exception {
f1(true);
}
/** F1 control: the same text without @CompileStatic. */
@Test
public void f1SignatureOnlyNestedTypeDynamic() throws Exception {
f1(false);
}
private void f1(boolean compileStatic) throws Exception {
writeHelper("class Helper { static Greeting make() { new Greeting() }\n"
+ "static class Greeting { String text() { 'hello' } } }\n");
write("op.groovy", "return Helper.make().text()\n");
final String text = (compileStatic ? "@groovy.transform.CompileStatic\n" : "")
+ "String use(boolean b) { b ? Helper.make().text() : 'not run' }\n"
+ "return use((boolean) useHelper)\n";
assertThat(runOnConnector(text, useHelper(false))).isEqualTo("not run");
editHelper("class Helper { static Greeting make() { new Greeting() }\n"
+ "static class Greeting { String text() { 'hi' } } }\n");
assertThat(configuration.evaluate("op.groovy", new Binding(), null)).isEqualTo("hi");
final Object result = outcome(() -> runOnConnector(text, useHelper(true)));
System.out.println("REVIEW-F1 compileStatic=" + compileStatic + " result: " + result);
assertThat(result).isIn("hello", "hi");
}
/** F2: one compilation of a text resolves Helper and Helper$Greeting from different versions. */
@Test(timeOut = 120000)
public void f2OneCompilationMixesTwoVersions() throws Exception {
configuration.release();
configuration = withScriptRoot(new ScriptedConfiguration() {
@Override
protected URL[] getRoots(CompilerConfiguration compilerConfiguration, GroovyClassLoader loader) {
final URL[] roots = super.getRoots(compilerConfiguration, loader);
final URL[] result = Arrays.copyOf(roots, roots.length + 1);
result[roots.length] = blockingRoot();
return result;
}
});
writeHelper("class Helper { static String greet() { 'hello' }\n"
+ "static class Greeting { static String text() { 'hello' } } }\n");
write("op.groovy", "return Helper.greet() + ' ' + Helper.Greeting.text()\n");
assertThat(configuration.evaluate("op.groovy", new Binding(), null)).isEqualTo("hello hello");
final String text = "def a = Helper.greet()\n"
+ "if (false) { Trigger.x() }\n"
+ "def b = Helper.Greeting.text()\n"
+ "return a + ' ' + b\n";
triggerWaiting = new CountDownLatch(1);
triggerGate = new CountDownLatch(1);
final Future<Object> textRun = executor.submit(
() -> runOnConnector(text, Collections.<String, Object>emptyMap()));
assertThat(triggerWaiting.await(30, SECONDS)).isTrue();
// The text has resolved Helper and is now looking up Trigger.
editHelper("class Helper { static String greet() { 'hi' }\n"
+ "static class Greeting { static String text() { 'hi' } } }\n");
final Future<Object> op = executor.submit(() -> configuration.evaluate("op.groovy", new Binding(), null));
Object opResult;
boolean opRanWhileTextCompiled;
try {
opResult = op.get(5, SECONDS);
opRanWhileTextCompiled = true;
} catch (TimeoutException e) {
opResult = null;
opRanWhileTextCompiled = false;
}
triggerGate.countDown();
if (!opRanWhileTextCompiled) {
opResult = op.get(30, SECONDS);
}
final Object result = outcome(() -> textRun.get(30, SECONDS));
System.out.println("REVIEW-F2 op ran while text compiled: " + opRanWhileTextCompiled
+ ", op: " + opResult + ", text: " + result);
assertThat(result).isIn("hello hello", "hi hi");
}
/** F3: what a subclass adds to the engine's configuration in getRoots. */
@Test
public void f3GetRootsConfigurationReachesTexts() throws Exception {
configuration.release();
configuration = withScriptRoot(new ScriptedConfiguration() {
@Override
protected URL[] getRoots(CompilerConfiguration compilerConfiguration, GroovyClassLoader loader) {
compilerConfiguration.addCompilationCustomizers(
new ImportCustomizer().addImport("RootsList", ArrayList.class.getName()));
final SecureASTCustomizer secure = new SecureASTCustomizer();
secure.setMethodDefinitionAllowed(false);
compilerConfiguration.addCompilationCustomizers(secure);
return super.getRoots(compilerConfiguration, loader);
}
});
write("op.groovy", "return new RootsList()\n");
final Object op = outcome(() -> configuration.evaluate("op.groovy", new Binding(), null));
final Object imports = outcome(() -> runOnConnector("return new RootsList()",
Collections.<String, Object>emptyMap()));
final Object secured = outcome(() -> runOnConnector("def f() { 1 }\nreturn f()",
Collections.<String, Object>emptyMap()));
System.out.println("REVIEW-F3 op: " + op + " | text import: " + imports + " | text method def: " + secured);
assertThat(imports).isInstanceOf(ArrayList.class);
assertThat(String.valueOf(secured)).startsWith("THREW");
}
/** F4: a URL a text adds to its class loader. */
@Test
public void f4UrlAddedByATextReachesTheEngine() throws Exception {
final File lib = Files.createTempDirectory("review-196-lib").toFile();
try {
final CompilerConfiguration libConfig = new CompilerConfiguration();
libConfig.setTargetDirectory(lib);
final CompilationUnit unit = new CompilationUnit(libConfig);
unit.addSource("Driver.groovy", "package drv\nclass Driver { static String id() { 'drv' } }\n");
unit.compile();
final URL libUrl = lib.toURI().toURL();
runOnConnector("this.class.classLoader.addURL(new URL('" + libUrl + "')); return null",
Collections.<String, Object>emptyMap());
final boolean inEngine = Arrays.asList(
configuration.getGroovyScriptEngine().getGroovyClassLoader().getURLs()).contains(libUrl);
write("op.groovy", "return this.class.classLoader.loadClass('drv.Driver').id()\n");
final Object op = outcome(() -> configuration.evaluate("op.groovy", new Binding(), null));
final Object otherText = outcome(() -> runOnConnector(
"return this.class.classLoader.loadClass('drv.Driver').id()",
Collections.<String, Object>emptyMap()));
System.out.println("REVIEW-F4 URL in engine loader: " + inEngine + " | op: " + op
+ " | other text: " + otherText);
assertThat(inEngine).isTrue();
} finally {
delete(lib);
}
}
private static URL blockingRoot() {
try {
return new URL(null, "blocking:/", new URLStreamHandler() {
@Override
protected URLConnection openConnection(final URL u) {
return new URLConnection(u) {
@Override
public void connect() throws IOException {
throw new FileNotFoundException(u.toString());
}
@Override
public InputStream getInputStream() throws IOException {
final CountDownLatch gate = triggerGate;
if ("/Trigger.groovy".equals(u.getPath()) && null != gate) {
triggerWaiting.countDown();
try {
gate.await();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
throw new FileNotFoundException(u.toString());
}
};
}
});
} catch (IOException e) {
throw new IllegalStateException(e);
}
}
private static Object outcome(Callable<Object> call) {
try {
return call.call();
} catch (Throwable t) {
Throwable e = t instanceof ExecutionException && null != t.getCause() ? t.getCause() : t;
final StringBuilder chain = new StringBuilder("THREW ");
for (int i = 0; null != e && i < 4; e = e.getCause(), i++) {
chain.append(i > 0 ? " <- " : "").append(e.getClass().getName()).append(": ")
.append(String.valueOf(e.getMessage()).replace('\n', ' '));
}
return chain.toString();
}
}
private Object runOnConnector(String text, Map<String, Object> arguments) {
final ScriptedPoolableConnector connector = new ScriptedPoolableConnector();
connector.init(configuration);
return connector.runScriptOnConnector(new ScriptContext("Groovy", text, arguments), null);
}
private static Map<String, Object> useHelper(boolean useHelper) {
return Collections.<String, Object>singletonMap("useHelper", useHelper);
}
private File write(String fileName, String text) throws IOException {
final File file = new File(scriptRoot, fileName);
file.getParentFile().mkdirs();
Files.write(file.toPath(), text.getBytes(StandardCharsets.UTF_8));
return file;
}
private void writeHelper(String text) throws IOException {
final File helper = write("Helper.groovy", text);
assertThat(helper.setLastModified(System.currentTimeMillis() - 60000L)).isTrue();
}
private void editHelper(String text) throws Exception {
Thread.sleep(200);
write("Helper.groovy", text);
Thread.sleep(200);
}
private ScriptedConfiguration withScriptRoot(ScriptedConfiguration result) {
result.setScriptRoots(new String[] { scriptRoot.getAbsolutePath() });
return result;
}
private static void delete(File file) {
final File[] children = file.listFiles();
if (null != children) {
for (File child : children) {
delete(child);
}
}
file.delete();
}
}2 participants
Fixes #165.
runScriptOnConnectorno longer compiles a new class into the script engine's loader on every call. Each distinct script text is compiled once per engine and kept in a bounded LRU cache. Classes that leave the cache, and every cached class onrelease(), are unregistered so that they can be unloaded. With the default size an engine keeps at most 64 runScriptOnConnector scripts, each with its closures and declared classes, however many calls it serves.ScriptedConfiguration.getScriptOnConnectorClasscompiles a text with aScriptOnConnectorLoader, a child of the engine's loader, which keeps the new classes out of the engine's class cache. Before, every call went throughgetGroovyScriptEngine().getGroovyClassLoader().parseClass(...). Each class then stayed in the engine loader, and inGroovyScriptEngine's script cache, untilrelease().Helper, resolves throughGroovyScriptEngine.loadScriptByName, as it does in the engine's own compilations. A text therefore uses theHelperthat the operation scripts use. The engine compiles it when it is first needed or after its source changes, and a script-root class still shadows a class of the connector's class loader. Other classes resolve through the engine's loader. The engine checks its script cache before it takes its compiler lock. A lock of the configuration is therefore held while a text looks up a class name in the script roots, found or not; without it, two texts that loadHelperfirst would get one copy each.ScriptOnConnectorLoaderkeeps every class the compiler resolved for the text and returns it fromloadClass. Without that, code that first ran after an operation script had recompiled an editedHelperlinked the new class, and@CompileStaticcode compiled against an old signature failed withNoSuchMethodError.ImportCustomizer. The compiler resolves the class nodes of the imports in place, and renames them while it tries inner class names. The engine's loader runs one compilation at a time, but texts compile in parallel. With the engine's customizer shared, the first texts compiled together on a new engine failed withunable to resolve class org$forgerock$openicf$misc$scriptedcommon$ICFObjectBuilder. The protected hooksgetImportCustomizer(ImportCustomizer)andgetImports()therefore run once per text compilation, possibly on several threads at once, where master called them once per engine. Their Javadoc now says so: an override returns a new customizer, or adds toparent, on every call.org.identityconnectors.framework.groovy.scriptOnConnectorCacheSizechanges the size. It is read when an engine is created. A value that is not a positive integer logs a WARN and falls back to 64.release()callInvokerHelper.removeClasson every class of the script's child loader, closures and declared classes included, outside the cache monitor. The cache is created with the engine once its customizer has succeeded.release()clears it last in itsfinally.ScriptedConnectorBase.runScriptOnConnectornow callsevaluateScriptOnConnector, which runsInvokerHelper.createScript(...).run()in Java. A Groovy call site caches the metaclass of the last script it ran, and so keeps that class alive. The arguments, thelogbinding and thecatchblocks are unchanged.getGroovyScriptEngine()without calling super has no cache either. A text then compiles through the engine's loader, with the engine's configuration, as on master, and is not cached. The configuration of an engine that a subclass builds, aSecureASTCustomizerfor example, therefore still applies to its texts.CompilationFailedExceptionreaches the caller, as on master.Behaviour changes:
Helperand their nested classes included, until it is evicted or the configuration is released. This holds even after their source changes on disk and the operation scripts move to the new classes. Master recompiled the text on every call and so picked up the change; a newly compiled text still does;release()goes into the orphaned cache, so its class is only softly held.Not changed:
loadScript,evaluate) are still compiled and cached by the engine;runScriptOnConnectormaps.Test strength
All in
ScriptOnConnectorCacheTest. The ed25f73 and ce3a86f cells are runs of the new tests against the production code of those earlier heads of this PR.#testSameTextReusesTheCompiledClass#testScriptClassStaysOutOfTheEngineLoadergetParentLoader(), not the engine's loader" killed#testScriptSeesTheDefaultImports#testScriptSeesTheClassesInTheScriptRoots#testScriptSharesScriptRootClassesWithOperationScripts#testNewScriptSharesTheEditedScriptRootClassWithOperationScripts#testScriptRootClassShadowsAClassOfTheConnectorLoader#testCachedScriptKeepsTheScriptRootClassItWasCompiledAgainst#testCachedScriptLinksTheScriptRootClassesItWasCompiledAgainsthi hi); mutants "noloadClassoverride", "keep only the script-root classes" and "child loader's parent isgetParentLoader()" killed#testScriptsCompiledTogetherShareTheScriptRootClasses#testScriptsCompileTogetherOnANewEngineunable to resolve class org$forgerock$openicf$misc$scriptedcommon$ICFObjectBuilder); mutant "child shares the engine's configuration" killed#testScriptSeesItsBindingrelease()unregisters and recompiles#testReleaseUnregistersTheCachedClasses#testReleaseUnregistersTheCachedClassesWhenTheReleaseClosureThrowstry/finally" killed#testCustomizerCanRunAScriptOnConnectorNullPointerException)#testScriptRunByTheCustomizerIsNotCached#testEngineBuiltByASubclassCompilesTheScriptWithItsOwnConfigurationunable to resolve class EngineList); mutants "no cache: compile with the child loader anyway" and "no null-cache guard" killed#testCachedScriptDoesNotWaitForAnotherCompilationsynchronized getScriptOnConnectorClasskilled (TimeoutException)5,7,0,-3,many#testCacheSizeComesFromTheSystemProperty#testLeastRecentlyUsedScriptIsEvictedaccessOrderfalsekilled#testEvictedScriptClassesAreUnregisteredput" killed#testEvictedScriptClassCanBeUnloaded#testConnectorReusesTheCompiledScriptlogbinding#testConnectorScriptSeesItsArgumentsAndTheLoggerlogisLog.getLog(Object.class)" and "evaluateScriptOnConnectorrebindslogto the script's class" killed#testScriptRunByTheConnectorCanBeUnloadedOnceEvictedcreateScript(...).run()killed (the call site keeps the class)#testBrokenScriptRootClassFailsAsACompilationErrorGroovyBugError); both resolver mutants killed#testConnectorReportsABrokenScriptRootClassAsACompilationErrorGroovyBugError); both resolver mutants killedThe two unload tests run
System.gc()for up to about 5 s and return as soon as the class is gone. They rely on the JVM's default soft-reference policy, which surefire leaves unchanged.Not pinned by a test:
release()(eviction pins the sharedunregister());mvn -pl OpenICF-groovy-connector,OpenICF-ssh-connector verifyon JDK 26:Tests run: 171, Failures: 0, Errors: 0, Skipped: 42. The 42 skipped tests are the five*SampleTestclasses, which need a private configuration;Tests run: 135, Failures: 0, Errors: 0, Skipped: 120. The skipped sample tests need a live host.#194 touches none of these files, and a trial merge is clean. With both merged, the groovy module runs 200 tests.