Skip to content

Commit

Permalink
Painless: Remove caching of Painless scripts in GenericElasticsearchS…
Browse files Browse the repository at this point in the history
…cript (#34116)

When a script uses the special-cased SearchScript or ExecutableScript, Painless is 
internally caching the GenericElasticsearchScript that is generated via compile as part of 
the newInstance method for the factories. This breaks the model for class bindings as the 
class binding expects to not have any stored information for each instance generated via 
newInstance from the factory. This change fixes the issue by creating a new instance of 
the GenericElasticsearchScript each time newInstance is called against the factory.
  • Loading branch information
jdconrad authored Sep 28, 2018
1 parent a984f8a commit bc84c8e
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 111 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

import java.lang.invoke.MethodType;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.AccessControlContext;
import java.security.AccessController;
Expand Down Expand Up @@ -127,25 +128,49 @@ public <T> T compile(String scriptName, String scriptSource, ScriptContext<T> co
Compiler compiler = contextsToCompilers.get(context);

if (context.instanceClazz.equals(SearchScript.class)) {
GenericElasticsearchScript painlessScript =
(GenericElasticsearchScript)compile(compiler, scriptName, scriptSource, params);
Constructor<?> constructor = compile(compiler, scriptName, scriptSource, params);
boolean needsScore;

try {
GenericElasticsearchScript newInstance = (GenericElasticsearchScript)constructor.newInstance();
needsScore = newInstance.needs_score();
} catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
throw new IllegalArgumentException("internal error");
}

SearchScript.Factory factory = (p, lookup) -> new SearchScript.LeafFactory() {
@Override
public SearchScript newInstance(final LeafReaderContext context) {
return new ScriptImpl(painlessScript, p, lookup, context);
try {
// a new instance is required for the class bindings model to work correctly
GenericElasticsearchScript newInstance = (GenericElasticsearchScript)constructor.newInstance();
return new ScriptImpl(newInstance, p, lookup, context);
} catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
throw new IllegalArgumentException("internal error");
}
}
@Override
public boolean needs_score() {
return painlessScript.needs_score();
return needsScore;
}
};
return context.factoryClazz.cast(factory);
} else if (context.instanceClazz.equals(ExecutableScript.class)) {
GenericElasticsearchScript painlessScript =
(GenericElasticsearchScript)compile(compiler, scriptName, scriptSource, params);
Constructor<?> constructor = compile(compiler, scriptName, scriptSource, params);

ExecutableScript.Factory factory = new ExecutableScript.Factory() {
@Override
public ExecutableScript newInstance(Map<String, Object> params) {
try {
// a new instance is required for the class bindings model to work correctly
GenericElasticsearchScript newInstance = (GenericElasticsearchScript)constructor.newInstance();
return new ScriptImpl(newInstance, params, null, null);
} catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
throw new IllegalArgumentException("internal error");
}
}
};

ExecutableScript.Factory factory = (p) -> new ScriptImpl(painlessScript, p, null, null);
return context.factoryClazz.cast(factory);
} else {
// Check we ourselves are not being called by unprivileged code.
Expand Down Expand Up @@ -367,7 +392,7 @@ private void writeNeedsMethods(Class<?> clazz, ClassWriter writer, MainMethodRes
}
}

Object compile(Compiler compiler, String scriptName, String source, Map<String, String> params, Object... args) {
Constructor<?> compile(Compiler compiler, String scriptName, String source, Map<String, String> params) {
final CompilerSettings compilerSettings = buildCompilerSettings(params);

// Check we ourselves are not being called by unprivileged code.
Expand All @@ -383,14 +408,14 @@ public Loader run() {

try {
// Drop all permissions to actually compile the code itself.
return AccessController.doPrivileged(new PrivilegedAction<Object>() {
return AccessController.doPrivileged(new PrivilegedAction<Constructor<?>>() {
@Override
public Object run() {
public Constructor<?> run() {
String name = scriptName == null ? source : scriptName;
Constructor<?> constructor = compiler.compile(loader, new MainMethodReserved(), name, source, compilerSettings);

try {
return constructor.newInstance(args);
return constructor;
} catch (Exception exception) { // Catch everything to let the user know this is something caused internally.
throw new IllegalStateException(
"An internal error occurred attempting to define the script [" + name + "].", exception);
Expand Down
Loading

0 comments on commit bc84c8e

Please sign in to comment.