You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2016/09/28 16:50:30 UTC
tinkerpop git commit: TINKERPOP-1478 Fixed memory leak and proper
redirection of output in GremlinGroovyScriptEngine
Repository: tinkerpop
Updated Branches:
refs/heads/tp31 14708fefc -> 762f6b229
TINKERPOP-1478 Fixed memory leak and proper redirection of output in GremlinGroovyScriptEngine
These were bugs identified in Groovy and fixed some time ago, but given that GremlinGroovyScriptEngine is based on that class and doesn't directly use it, those fixes were never in place for it. CTR
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/762f6b22
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/762f6b22
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/762f6b22
Branch: refs/heads/tp31
Commit: 762f6b229925d407390e78d587ef98863205c870
Parents: 14708fe
Author: Stephen Mallette <sp...@genoprime.com>
Authored: Wed Sep 28 12:48:26 2016 -0400
Committer: Stephen Mallette <sp...@genoprime.com>
Committed: Wed Sep 28 12:48:26 2016 -0400
----------------------------------------------------------------------
CHANGELOG.asciidoc | 1 +
.../src/reference/gremlin-applications.asciidoc | 25 +++-
.../jsr223/GremlinGroovyScriptEngine.java | 140 ++++++++++---------
.../jsr223/GremlinGroovyScriptEngineTest.java | 69 ++++++++-
4 files changed, 162 insertions(+), 73 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/762f6b22/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 4b39cc7..d0aa8e8 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
TinkerPop 3.1.5 (Release Date: NOT OFFICIALLY RELEASED YET)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+* Fixed output redirection and potential memory leak in `GremlinGroovyScriptEngine`.
* Corrected naming of `g_withPath_V_asXaX_out_out_mapXa_name_it_nameX` and `g_withPath_V_asXaX_out_mapXa_nameX` in `MapTest`.
* Improved session cleanup when a close is triggered by the client.
* Removed the `appveyor.yml` file as the AppVeyor build is no longer enabled by Apache Infrastructure.
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/762f6b22/docs/src/reference/gremlin-applications.asciidoc
----------------------------------------------------------------------
diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc
index cedd98f..d8b891e 100644
--- a/docs/src/reference/gremlin-applications.asciidoc
+++ b/docs/src/reference/gremlin-applications.asciidoc
@@ -1309,12 +1309,12 @@ client.submit("[1,2,3,x]", params);
Cache Management
^^^^^^^^^^^^^^^^
-If Gremlin Server processes a large number of unique scripts, the cache will grow beyond the memory available to
-Gremlin Server and an `OutOfMemoryError` will loom. Script parameterization goes a long way to solving this problem
-and running out of memory should not be an issue for those cases. If it is a problem or if there is no script
-parameterization due to a given use case (perhaps using with use of <<sessions,sessions>>), it is possible to better
-control the nature of the script cache from the client side, by issuing scripts with a parameter to help define how
-the garbage collector should treat the references.
+If Gremlin Server processes a large number of unique scripts, the global function cache will grow beyond the memory
+available to Gremlin Server and an `OutOfMemoryError` will loom. Script parameterization goes a long way to solving
+this problem and running out of memory should not be an issue for those cases. If it is a problem or if there is no
+script parameterization due to a given use case (perhaps using with use of <<sessions,sessions>>), it is possible to
+better control the nature of the global function cache from the client side, by issuing scripts with a parameter to
+help define how the garbage collector should treat the references.
The parameter is called `#jsr223.groovy.engine.keep.globals` and has four options:
@@ -1324,9 +1324,20 @@ The parameter is called `#jsr223.groovy.engine.keep.globals` and has four option
* `phantom` - removed immediately after being evaluated by the `ScriptEngine`.
By specifying an option other than `hard`, an `OutOfMemoryError` in Gremlin Server should be avoided. Of course,
-this approach will come with the downside that compiled scripts could be garbage collected and thus removed from the
+this approach will come with the downside that functions could be garbage collected and thus removed from the
cache, forcing Gremlin Server to recompile later if that script is later encountered.
+[source,java]
+----
+Cluster cluster = Cluster.open();
+Client client = cluster.connect();
+
+Map<String,Object> params = new HashMap<>();
+params.put("x",4);
+params.put("#jsr223.groovy.engine.keep.globals", "soft");
+client.submit("[1,2,3,x]", params);
+----
+
[[sessions]]
Considering Sessions
^^^^^^^^^^^^^^^^^^^^
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/762f6b22/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java
----------------------------------------------------------------------
diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java
index ca129c6..acc7f90 100644
--- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java
+++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngine.java
@@ -46,7 +46,6 @@ import org.codehaus.groovy.jsr223.GroovyScriptEngineImpl;
import org.codehaus.groovy.runtime.InvokerHelper;
import org.codehaus.groovy.runtime.MetaClassHelper;
import org.codehaus.groovy.runtime.MethodClosure;
-import org.codehaus.groovy.syntax.SyntaxException;
import org.codehaus.groovy.util.ReferenceBundle;
import javax.script.Bindings;
@@ -385,8 +384,6 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements
final Class clazz = getScriptClass(script);
if (null == clazz) throw new ScriptException("Script class is null");
return eval(clazz, context);
- } catch (SyntaxException e) {
- throw new ScriptException(e.getMessage(), e.getSourceLocator(), e.getLine());
} catch (Exception e) {
throw new ScriptException(e);
}
@@ -422,9 +419,7 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements
public CompiledScript compile(final String scriptSource) throws ScriptException {
try {
return new GroovyCompiledScript(this, getScriptClass(scriptSource));
- } catch (SyntaxException e) {
- throw new ScriptException(e.getMessage(), e.getSourceLocator(), e.getLine());
- } catch (IOException | CompilationFailedException e) {
+ } catch (CompilationFailedException e) {
throw new ScriptException(e);
}
}
@@ -463,7 +458,7 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements
return makeInterface(thiz, clazz);
}
- Class getScriptClass(final String script) throws SyntaxException, CompilationFailedException, IOException {
+ Class getScriptClass(final String script) throws CompilationFailedException {
Class clazz = classMap.get(script);
if (clazz != null) return clazz;
@@ -477,23 +472,33 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements
}
Object eval(final Class scriptClass, final ScriptContext context) throws ScriptException {
- context.setAttribute("context", context, ScriptContext.ENGINE_SCOPE);
- final Writer writer = context.getWriter();
- context.setAttribute("out", writer instanceof PrintWriter ? writer : new PrintWriter(writer), ScriptContext.ENGINE_SCOPE);
- final Binding binding = new Binding() {
+ final Binding binding = new Binding(context.getBindings(ScriptContext.ENGINE_SCOPE)) {
@Override
- public Object getVariable(final String name) {
+ public Object getVariable(String name) {
synchronized (context) {
- final int scope = context.getAttributesScope(name);
+ int scope = context.getAttributesScope(name);
if (scope != -1) {
return context.getAttribute(name, scope);
}
- throw new MissingPropertyException(name, getClass());
+ // Redirect script output to context writer, if out var is not already provided
+ if ("out".equals(name)) {
+ final Writer writer = context.getWriter();
+ if (writer != null) {
+ return (writer instanceof PrintWriter) ?
+ (PrintWriter) writer :
+ new PrintWriter(writer, true);
+ }
+ }
+ // Provide access to engine context, if context var is not already provided
+ if ("context".equals(name)) {
+ return context;
+ }
}
+ throw new MissingPropertyException(name, getClass());
}
@Override
- public void setVariable(final String name, final Object value) {
+ public void setVariable(String name, Object value) {
synchronized (context) {
int scope = context.getAttributesScope(name);
if (scope == -1) {
@@ -505,67 +510,72 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements
};
try {
- final Script scriptObject = InvokerHelper.createScript(scriptClass, binding);
- for (Method m : scriptClass.getMethods()) {
- final String name = m.getName();
- globalClosures.put(name, new MethodClosure(scriptObject, name));
- }
+ // if this class is not an instance of Script, it's a full-blown class then simply return that class
+ if (!Script.class.isAssignableFrom(scriptClass)) {
+ return scriptClass;
+ } else {
+ final Script scriptObject = InvokerHelper.createScript(scriptClass, binding);
+ for (Method m : scriptClass.getMethods()) {
+ final String name = m.getName();
+ globalClosures.put(name, new MethodClosure(scriptObject, name));
+ }
- final MetaClass oldMetaClass = scriptObject.getMetaClass();
- scriptObject.setMetaClass(new DelegatingMetaClass(oldMetaClass) {
- @Override
- public Object invokeMethod(final Object object, final String name, final Object args) {
- if (args == null) {
- return invokeMethod(object, name, MetaClassHelper.EMPTY_ARRAY);
- } else if (args instanceof Tuple) {
- return invokeMethod(object, name, ((Tuple) args).toArray());
- } else if (args instanceof Object[]) {
- return invokeMethod(object, name, (Object[]) args);
- } else {
- return invokeMethod(object, name, new Object[]{args});
+ final MetaClass oldMetaClass = scriptObject.getMetaClass();
+ scriptObject.setMetaClass(new DelegatingMetaClass(oldMetaClass) {
+ @Override
+ public Object invokeMethod(final Object object, final String name, final Object args) {
+ if (args == null) {
+ return invokeMethod(object, name, MetaClassHelper.EMPTY_ARRAY);
+ } else if (args instanceof Tuple) {
+ return invokeMethod(object, name, ((Tuple) args).toArray());
+ } else if (args instanceof Object[]) {
+ return invokeMethod(object, name, (Object[]) args);
+ } else {
+ return invokeMethod(object, name, new Object[]{args});
+ }
}
- }
- @Override
- public Object invokeMethod(final Object object, final String name, final Object args[]) {
- try {
- return super.invokeMethod(object, name, args);
- } catch (MissingMethodException mme) {
- return callGlobal(name, args, context);
+ @Override
+ public Object invokeMethod(final Object object, final String name, final Object args[]) {
+ try {
+ return super.invokeMethod(object, name, args);
+ } catch (MissingMethodException mme) {
+ return callGlobal(name, args, context);
+ }
}
- }
- @Override
- public Object invokeStaticMethod(final Object object, final String name, final Object args[]) {
- try {
- return super.invokeStaticMethod(object, name, args);
- } catch (MissingMethodException mme) {
- return callGlobal(name, args, context);
+ @Override
+ public Object invokeStaticMethod(final Object object, final String name, final Object args[]) {
+ try {
+ return super.invokeStaticMethod(object, name, args);
+ } catch (MissingMethodException mme) {
+ return callGlobal(name, args, context);
+ }
}
- }
- });
+ });
- final Object o = scriptObject.run();
+ final Object o = scriptObject.run();
- // if interpreter mode is enable then local vars of the script are promoted to engine scope bindings.
- if (interpreterModeEnabled) {
- final Map<String, Object> localVars = (Map<String, Object>) context.getAttribute(COLLECTED_BOUND_VARS_MAP_VARNAME);
- if (localVars != null) {
- localVars.entrySet().forEach(e -> {
- // closures need to be cached for later use
- if (e.getValue() instanceof Closure)
- globalClosures.put(e.getKey(), (Closure) e.getValue());
+ // if interpreter mode is enable then local vars of the script are promoted to engine scope bindings.
+ if (interpreterModeEnabled) {
+ final Map<String, Object> localVars = (Map<String, Object>) context.getAttribute(COLLECTED_BOUND_VARS_MAP_VARNAME);
+ if (localVars != null) {
+ localVars.entrySet().forEach(e -> {
+ // closures need to be cached for later use
+ if (e.getValue() instanceof Closure)
+ globalClosures.put(e.getKey(), (Closure) e.getValue());
- context.setAttribute(e.getKey(), e.getValue(), ScriptContext.ENGINE_SCOPE);
- });
+ context.setAttribute(e.getKey(), e.getValue(), ScriptContext.ENGINE_SCOPE);
+ });
- // get rid of the temporary collected vars
- context.removeAttribute(COLLECTED_BOUND_VARS_MAP_VARNAME, ScriptContext.ENGINE_SCOPE);
- localVars.clear();
+ // get rid of the temporary collected vars
+ context.removeAttribute(COLLECTED_BOUND_VARS_MAP_VARNAME, ScriptContext.ENGINE_SCOPE);
+ localVars.clear();
+ }
}
- }
- return o;
+ return o;
+ }
} catch (Exception e) {
throw new ScriptException(e);
}
@@ -607,7 +617,7 @@ public class GremlinGroovyScriptEngine extends GroovyScriptEngineImpl implements
}
private synchronized void createClassLoader() {
- final CompilerConfiguration conf = new CompilerConfiguration();
+ final CompilerConfiguration conf = new CompilerConfiguration(CompilerConfiguration.DEFAULT);
conf.addCompilationCustomizers(this.importCustomizerProvider.create());
customizerProviders.forEach(p -> conf.addCompilationCustomizers(p.create()));
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/762f6b22/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java
----------------------------------------------------------------------
diff --git a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java
index 19ced88..b18c020 100644
--- a/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java
+++ b/gremlin-groovy/src/test/java/org/apache/tinkerpop/gremlin/groovy/jsr223/GremlinGroovyScriptEngineTest.java
@@ -38,13 +38,15 @@ import javax.script.ScriptEngine;
import javax.script.ScriptException;
import javax.script.SimpleBindings;
import java.awt.*;
+import java.io.StringWriter;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
+import java.util.Map;
import java.util.Set;
-import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -71,6 +73,8 @@ import static org.junit.Assert.fail;
public class GremlinGroovyScriptEngineTest {
private static final Logger logger = LoggerFactory.getLogger(GremlinGroovyScriptEngineTest.class);
+ private static final Object[] EMPTY_ARGS = new Object[0];
+
@Test
public void shouldCompileScriptWithoutRequiringVariableBindings() throws Exception {
// compile() should cache the script to avoid future compilation
@@ -426,4 +430,67 @@ public class GremlinGroovyScriptEngineTest {
assertEquals(t.getValue0() * -1, t.getValue1().get(2).intValue());
});
}
+
+ @Test
+ public void shouldInvokeFunctionRedirectsOutputToContextWriter() throws Exception {
+ final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine();
+ StringWriter writer = new StringWriter();
+ engine.getContext().setWriter(writer);
+
+ final String code = "def myFunction() { print \"Hello World!\" }";
+ engine.eval(code);
+ engine.invokeFunction("myFunction", EMPTY_ARGS);
+ assertEquals("Hello World!", writer.toString());
+
+ writer = new StringWriter();
+ final StringWriter writer2 = new StringWriter();
+ engine.getContext().setWriter(writer2);
+ engine.invokeFunction("myFunction", EMPTY_ARGS);
+ assertEquals("", writer.toString());
+ assertEquals("Hello World!", writer2.toString());
+ }
+
+ @Test
+ public void testInvokeFunctionRedirectsOutputToContextOut() throws Exception {
+ final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine();
+ StringWriter writer = new StringWriter();
+ final StringWriter unusedWriter = new StringWriter();
+ engine.getContext().setWriter(unusedWriter);
+ engine.put("out", writer);
+
+ final String code = "def myFunction() { print \"Hello World!\" }";
+ engine.eval(code);
+ engine.invokeFunction("myFunction", EMPTY_ARGS);
+ assertEquals("", unusedWriter.toString());
+ assertEquals("Hello World!", writer.toString());
+
+ writer = new StringWriter();
+ final StringWriter writer2 = new StringWriter();
+ engine.put("out", writer2);
+ engine.invokeFunction("myFunction", EMPTY_ARGS);
+ assertEquals("", unusedWriter.toString());
+ assertEquals("", writer.toString());
+ assertEquals("Hello World!", writer2.toString());
+ }
+
+ @Test
+ public void testEngineContextAccessibleToScript() throws Exception {
+ final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine();
+ final ScriptContext engineContext = engine.getContext();
+ engine.put("theEngineContext", engineContext);
+ final String code = "[answer: theEngineContext.is(context)]";
+ assertThat(((Map) engine.eval(code)).get("answer"), is(true));
+ }
+
+ @Test
+ public void testContextBindingOverridesEngineContext() throws Exception {
+ final GremlinGroovyScriptEngine engine = new GremlinGroovyScriptEngine();
+ final ScriptContext engineContext = engine.getContext();
+ final Map<String,Object> otherContext = new HashMap<>();
+ otherContext.put("foo", "bar");
+ engine.put("context", otherContext);
+ engine.put("theEngineContext", engineContext);
+ final String code = "[answer: context.is(theEngineContext) ? \"wrong\" : context.foo]";
+ assertEquals("bar", ((Map) engine.eval(code)).get("answer"));
+ }
}
\ No newline at end of file