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 19:21:42 UTC
[1/4] tinkerpop git commit: fixed conf dir references [Forced Update!]
Repository: tinkerpop
Updated Branches:
refs/heads/TINKERPOP-1467 934054f83 -> c3acce6e0 (forced update)
fixed conf dir references
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/3cbd3719
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/3cbd3719
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/3cbd3719
Branch: refs/heads/TINKERPOP-1467
Commit: 3cbd3719084e5dcbb9b470228bd6a40e6d050478
Parents: 33225c5
Author: Robert Dale <ro...@gmail.com>
Authored: Mon Sep 26 11:01:53 2016 -0400
Committer: Robert Dale <ro...@gmail.com>
Committed: Tue Sep 27 20:42:15 2016 -0400
----------------------------------------------------------------------
docs/src/reference/gremlin-applications.asciidoc | 4 ++--
gremlin-console/src/main/bin/gremlin.sh | 4 ++--
gremlin-driver/src/main/bin/config-eval.sh | 4 ++--
gremlin-driver/src/main/bin/profile-driver.sh | 4 ++--
gremlin-server/src/main/bin/gremlin-server.sh | 4 ++--
5 files changed, 10 insertions(+), 10 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3cbd3719/docs/src/reference/gremlin-applications.asciidoc
----------------------------------------------------------------------
diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc
index cedd98f..7e827f1 100644
--- a/docs/src/reference/gremlin-applications.asciidoc
+++ b/docs/src/reference/gremlin-applications.asciidoc
@@ -775,7 +775,7 @@ Configuring
~~~~~~~~~~~
As mentioned earlier, Gremlin Server is configured though a YAML file. By default, Gremlin Server will look for a
-file called `config/gremlin-server.yaml` to configure itself on startup. To override this default, supply the file
+file called `conf/gremlin-server.yaml` to configure itself on startup. To override this default, supply the file
to use to `bin/gremlin-server.sh` as in:
[source,text]
@@ -920,7 +920,7 @@ Cluster cluster = Cluster.build().credentials("stephen", "password")
.enableSsl(true).create();
If connecting with Gremlin Console, which utilizes `gremlin-driver` for remote script execution, use the provided
-`config/remote-secure.yaml` file when defining the remote. That file contains configuration for the username and
+`conf/remote-secure.yaml` file when defining the remote. That file contains configuration for the username and
password as well as enablement of SSL from the client side.
Similarly, Gremlin Server can be configured for REST and security.
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3cbd3719/gremlin-console/src/main/bin/gremlin.sh
----------------------------------------------------------------------
diff --git a/gremlin-console/src/main/bin/gremlin.sh b/gremlin-console/src/main/bin/gremlin.sh
index 84ba8179..fe28773 100755
--- a/gremlin-console/src/main/bin/gremlin.sh
+++ b/gremlin-console/src/main/bin/gremlin.sh
@@ -36,11 +36,11 @@ fi
case `uname` in
CYGWIN*)
- CP="`dirname $0`"/../config
+ CP="`dirname $0`"/../conf
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /;/g')
;;
*)
- CP="`dirname $0`"/../config
+ CP="`dirname $0`"/../conf
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /:/g')
esac
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3cbd3719/gremlin-driver/src/main/bin/config-eval.sh
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/bin/config-eval.sh b/gremlin-driver/src/main/bin/config-eval.sh
index c55ebf5..764d732 100644
--- a/gremlin-driver/src/main/bin/config-eval.sh
+++ b/gremlin-driver/src/main/bin/config-eval.sh
@@ -20,11 +20,11 @@
#
case `uname` in
CYGWIN*)
- CP="`dirname $0`"/../config/
+ CP="`dirname $0`"/../conf/
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /;/g')
;;
*)
- CP="`dirname $0`"/../config/
+ CP="`dirname $0`"/../conf/
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /:/g')
esac
#echo $CP
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3cbd3719/gremlin-driver/src/main/bin/profile-driver.sh
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/bin/profile-driver.sh b/gremlin-driver/src/main/bin/profile-driver.sh
index ce747e5..7b4ac1b 100644
--- a/gremlin-driver/src/main/bin/profile-driver.sh
+++ b/gremlin-driver/src/main/bin/profile-driver.sh
@@ -20,11 +20,11 @@
#
case `uname` in
CYGWIN*)
- CP="`dirname $0`"/../config/
+ CP="`dirname $0`"/../conf/
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /;/g')
;;
*)
- CP="`dirname $0`"/../config/
+ CP="`dirname $0`"/../conf/
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /:/g')
esac
#echo $CP
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/3cbd3719/gremlin-server/src/main/bin/gremlin-server.sh
----------------------------------------------------------------------
diff --git a/gremlin-server/src/main/bin/gremlin-server.sh b/gremlin-server/src/main/bin/gremlin-server.sh
index a82f73f..fcede2b 100755
--- a/gremlin-server/src/main/bin/gremlin-server.sh
+++ b/gremlin-server/src/main/bin/gremlin-server.sh
@@ -20,11 +20,11 @@
#
case `uname` in
CYGWIN*)
- CP="`dirname $0`"/../config/
+ CP="`dirname $0`"/../conf/
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /;/g')
;;
*)
- CP="`dirname $0`"/../config/
+ CP="`dirname $0`"/../conf/
CP="$CP":$( echo `dirname $0`/../lib/*.jar . | sed 's/ /:/g')
esac
#echo $CP
[3/4] tinkerpop git commit: Merge branch 'pr-438' into tp31
Posted by sp...@apache.org.
Merge branch 'pr-438' into tp31
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/bdef1a4c
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/bdef1a4c
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/bdef1a4c
Branch: refs/heads/TINKERPOP-1467
Commit: bdef1a4c281c328bc25bc9168f442145360a7eb1
Parents: 762f6b2 3cbd371
Author: Stephen Mallette <sp...@genoprime.com>
Authored: Wed Sep 28 12:53:46 2016 -0400
Committer: Stephen Mallette <sp...@genoprime.com>
Committed: Wed Sep 28 12:53:46 2016 -0400
----------------------------------------------------------------------
docs/src/reference/gremlin-applications.asciidoc | 4 ++--
gremlin-console/src/main/bin/gremlin.sh | 4 ++--
gremlin-driver/src/main/bin/config-eval.sh | 4 ++--
gremlin-driver/src/main/bin/profile-driver.sh | 4 ++--
gremlin-server/src/main/bin/gremlin-server.sh | 4 ++--
5 files changed, 10 insertions(+), 10 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/bdef1a4c/docs/src/reference/gremlin-applications.asciidoc
----------------------------------------------------------------------
[2/4] tinkerpop git commit: TINKERPOP-1478 Fixed memory leak and
proper redirection of output in GremlinGroovyScriptEngine
Posted by sp...@apache.org.
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/TINKERPOP-1467
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
[4/4] tinkerpop git commit: Corrected a number of problems in close()
operations for the driver.
Posted by sp...@apache.org.
Corrected a number of problems in close() operations for the driver.
This was more of a commit than I wanted for tp31, but close() was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the Cluster makes an attempt to clean up any Client instances that it spawns.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo
Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/c3acce6e
Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/c3acce6e
Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/c3acce6e
Branch: refs/heads/TINKERPOP-1467
Commit: c3acce6e0a8c9270a10ed0eb4f3fd3f46539a655
Parents: bdef1a4
Author: Stephen Mallette <sp...@genoprime.com>
Authored: Wed Sep 28 11:06:14 2016 -0400
Committer: Stephen Mallette <sp...@genoprime.com>
Committed: Wed Sep 28 15:21:11 2016 -0400
----------------------------------------------------------------------
CHANGELOG.asciidoc | 1 +
.../upgrade/release-3.1.x-incubating.asciidoc | 17 +++++
.../tinkerpop/gremlin/driver/Channelizer.java | 6 +-
.../apache/tinkerpop/gremlin/driver/Client.java | 42 +++++++++--
.../tinkerpop/gremlin/driver/Cluster.java | 22 +++++-
.../tinkerpop/gremlin/driver/Connection.java | 57 +++++++++++++--
.../gremlin/driver/ConnectionPool.java | 21 +++---
.../tinkerpop/gremlin/driver/Handler.java | 27 +++++--
.../tinkerpop/gremlin/driver/ResultQueue.java | 4 ++
.../driver/handler/WebSocketClientHandler.java | 4 +-
.../server/GremlinDriverIntegrateTest.java | 75 +++++++++++++++++++-
.../server/GremlinServerAuthIntegrateTest.java | 5 +-
.../GremlinServerAuthOldIntegrateTest.java | 4 +-
.../GremlinServerSessionIntegrateTest.java | 6 +-
14 files changed, 243 insertions(+), 48 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/CHANGELOG.asciidoc
----------------------------------------------------------------------
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index d0aa8e8..9f45477 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)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+* Improved handling of `Cluster.close()` and `Client.close()` to prevent the methods from hanging.
* 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.
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/docs/src/upgrade/release-3.1.x-incubating.asciidoc
----------------------------------------------------------------------
diff --git a/docs/src/upgrade/release-3.1.x-incubating.asciidoc b/docs/src/upgrade/release-3.1.x-incubating.asciidoc
index 400ea10..a2b8b53 100644
--- a/docs/src/upgrade/release-3.1.x-incubating.asciidoc
+++ b/docs/src/upgrade/release-3.1.x-incubating.asciidoc
@@ -27,6 +27,23 @@ TinkerPop 3.1.5
*Release Date: NOT OFFICIALLY RELEASED YET*
+Please see the link:https://github.com/apache/tinkerpop/blob/3.1.4/CHANGELOG.asciidoc#tinkerpop-315-release-date-XXXXXXXXXXXX[changelog] for a complete list of all the modifications that are part of this release.
+
+Upgrading for Users
+~~~~~~~~~~~~~~~~~~~
+
+Java Driver and close()
+^^^^^^^^^^^^^^^^^^^^^^^
+
+There were a few problems noted around the `close()` of `Cluster` and `Client` instances, including issues that
+presented as system hangs. These issues have been resolved, however, it is worth nothing that an unchecked exception
+that was thrown under a certain situation has changed as part of the bug fixes. When submitting an in-session request
+on a `Client` that was closed (or closing) an `IllegalStateException` is thrown. This replaces older functionality
+that threw a `ConnectionException` and relied logic far deeper in the driver to produce that error and had the
+potential to open additional resources despite the intention of the user to "close".
+
+See: https://issues.apache.org/jira/browse/TINKERPOP-1467[TINKERPOP-1467]
+
TinkerPop 3.1.4
---------------
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java
index 40be11c..b3761b7 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java
@@ -20,9 +20,6 @@ package org.apache.tinkerpop.gremlin.driver;
import io.netty.channel.Channel;
import io.netty.handler.codec.http.websocketx.CloseWebSocketFrame;
-import io.netty.handler.ssl.SslContextBuilder;
-import io.netty.handler.ssl.SslProvider;
-import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
import org.apache.tinkerpop.gremlin.driver.exception.ConnectionException;
import org.apache.tinkerpop.gremlin.driver.handler.NioGremlinRequestEncoder;
import org.apache.tinkerpop.gremlin.driver.handler.NioGremlinResponseDecoder;
@@ -42,7 +39,6 @@ import io.netty.handler.ssl.SslContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.io.File;
import java.util.Optional;
import java.util.UUID;
import java.util.concurrent.ConcurrentMap;
@@ -156,7 +152,7 @@ public interface Channelizer extends ChannelHandler {
*/
@Override
public void close(final Channel channel) {
- channel.writeAndFlush(new CloseWebSocketFrame());
+ if (channel.isOpen()) channel.writeAndFlush(new CloseWebSocketFrame());
}
@Override
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java
index 4aca9ca..3a03141 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java
@@ -36,6 +36,7 @@ import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
/**
@@ -245,6 +246,8 @@ public abstract class Client {
* A low-level method that allows the submission of a manually constructed {@link RequestMessage}.
*/
public CompletableFuture<ResultSet> submitAsync(final RequestMessage msg) {
+ if (isClosing()) throw new IllegalStateException("Client has been closed");
+
if (!initialized)
init();
@@ -269,6 +272,8 @@ public abstract class Client {
}
}
+ public abstract boolean isClosing();
+
/**
* Closes the client by making a synchronous call to {@link #closeAsync()}.
*/
@@ -290,11 +295,17 @@ public abstract class Client {
public final static class ClusteredClient extends Client {
private ConcurrentMap<Host, ConnectionPool> hostConnectionPools = new ConcurrentHashMap<>();
+ private final AtomicReference<CompletableFuture<Void>> closing = new AtomicReference<>(null);
ClusteredClient(final Cluster cluster) {
super(cluster);
}
+ @Override
+ public boolean isClosing() {
+ return closing.get() != null;
+ }
+
/**
* Submits a Gremlin script to the server and returns a {@link ResultSet} once the write of the request is
* complete.
@@ -364,10 +375,14 @@ public abstract class Client {
* Closes all the connection pools on all hosts.
*/
@Override
- public CompletableFuture<Void> closeAsync() {
+ public synchronized CompletableFuture<Void> closeAsync() {
+ if (closing.get() != null)
+ return closing.get();
+
final CompletableFuture[] poolCloseFutures = new CompletableFuture[hostConnectionPools.size()];
hostConnectionPools.values().stream().map(ConnectionPool::closeAsync).collect(Collectors.toList()).toArray(poolCloseFutures);
- return CompletableFuture.allOf(poolCloseFutures);
+ closing.set(CompletableFuture.allOf(poolCloseFutures));
+ return closing.get();
}
}
@@ -448,11 +463,16 @@ public abstract class Client {
* close on the {@code Client} that created it.
*/
@Override
- public CompletableFuture<Void> closeAsync() {
+ public synchronized CompletableFuture<Void> closeAsync() {
close.complete(null);
return close;
}
+ @Override
+ public boolean isClosing() {
+ return close.isDone();
+ }
+
/**
* {@inheritDoc}
*/
@@ -483,6 +503,8 @@ public abstract class Client {
private ConnectionPool connectionPool;
+ private final AtomicReference<CompletableFuture<Void>> closing = new AtomicReference<>(null);
+
SessionedClient(final Cluster cluster, final String sessionId, final boolean manageTransactions) {
super(cluster);
this.sessionId = sessionId;
@@ -526,12 +548,22 @@ public abstract class Client {
connectionPool = new ConnectionPool(host, this, Optional.of(1), Optional.of(1));
}
+ @Override
+ public boolean isClosing() {
+ return closing.get() != null;
+ }
+
/**
* Close the bound {@link ConnectionPool}.
*/
@Override
- public CompletableFuture<Void> closeAsync() {
- return connectionPool.closeAsync();
+ public synchronized CompletableFuture<Void> closeAsync() {
+ if (closing.get() != null)
+ return closing.get();
+
+ final CompletableFuture<Void> connectionPoolClose = connectionPool.closeAsync();
+ closing.set(connectionPoolClose);
+ return connectionPoolClose;
}
}
}
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java
index 6a6a2e3..473991a 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java
@@ -36,6 +36,7 @@ import javax.net.ssl.TrustManager;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
+import java.lang.ref.WeakReference;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.URI;
@@ -83,7 +84,9 @@ public final class Cluster {
* submitted or can be directly initialized via {@link Client#init()}.
*/
public <T extends Client> T connect() {
- return (T) new Client.ClusteredClient(this);
+ final Client client = new Client.ClusteredClient(this);
+ manager.trackClient(client);
+ return (T) client;
}
/**
@@ -122,7 +125,9 @@ public final class Cluster {
public <T extends Client> T connect(final String sessionId, final boolean manageTransactions) {
if (null == sessionId || sessionId.isEmpty())
throw new IllegalArgumentException("sessionId cannot be null or empty");
- return (T) new Client.SessionedClient(this, sessionId, manageTransactions);
+ final Client client = new Client.SessionedClient(this, sessionId, manageTransactions);
+ manager.trackClient(client);
+ return (T) client;
}
@Override
@@ -684,6 +689,8 @@ public final class Cluster {
private final AtomicReference<CompletableFuture<Void>> closeFuture = new AtomicReference<>();
+ private final List<WeakReference<Client>> openedClients = new ArrayList<>();
+
private Manager(final Builder builder) {
this.loadBalancingStrategy = builder.loadBalancingStrategy;
this.authProps = builder.authProps;
@@ -730,6 +737,10 @@ public final class Cluster {
});
}
+ void trackClient(final Client client) {
+ openedClients.add(new WeakReference<>(client));
+ }
+
public Host add(final InetSocketAddress address) {
final Host newHost = new Host(address, Cluster.this);
final Host previous = hosts.putIfAbsent(address, newHost);
@@ -745,6 +756,13 @@ public final class Cluster {
if (closeFuture.get() != null)
return closeFuture.get();
+ for (WeakReference<Client> openedClient : openedClients) {
+ final Client client = openedClient.get();
+ if (client != null && !client.isClosing()) {
+ client.close();
+ }
+ }
+
final CompletableFuture<Void> closeIt = new CompletableFuture<>();
closeFuture.set(closeIt);
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
index 220ad42..766db2e 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
@@ -35,6 +35,8 @@ import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;
@@ -148,22 +150,26 @@ final class Connection {
return pending;
}
- public CompletableFuture<Void> closeAsync() {
+ public synchronized CompletableFuture<Void> closeAsync() {
+ if (isClosed()) return closeFuture.get();
+
final CompletableFuture<Void> future = new CompletableFuture<>();
- if (!closeFuture.compareAndSet(null, future))
- return closeFuture.get();
+ closeFuture.set(future);
// make sure all requests in the queue are fully processed before killing. if they are then shutdown
// can be immediate. if not this method will signal the readCompleted future defined in the write()
// operation to check if it can close. in this way the connection no longer receives writes, but
// can continue to read. If a request never comes back the future won't get fulfilled and the connection
// will maintain a "pending" request, that won't quite ever go away. The build up of such a dead requests
- // on a connection in the connection pool will force the pool to replace the connection for a fresh one
+ // on a connection in the connection pool will force the pool to replace the connection for a fresh one.
if (pending.isEmpty()) {
if (null == channel)
future.complete(null);
else
shutdown(future);
+ } else {
+ // there may be some pending requests. schedule a job to wait for those to complete and then shutdown
+ new CheckForPending(future).runUntilDone(cluster.executor(), 1000, TimeUnit.MILLISECONDS);
}
return future;
@@ -256,7 +262,7 @@ final class Connection {
shutdown(closeFuture.get());
}
- private void shutdown(final CompletableFuture<Void> future) {
+ private synchronized void shutdown(final CompletableFuture<Void> future) {
// shutdown can be called directly from closeAsync() or after write() and therefore this method should only
// be called once. once shutdown is initiated, it shouldn't be executed a second time or else it sends more
// messages at the server and leads to ugly log messages over there.
@@ -286,6 +292,7 @@ final class Connection {
}
channelizer.close(channel);
+
final ChannelPromise promise = channel.newPromise();
promise.addListener(f -> {
if (f.cause() != null)
@@ -307,4 +314,44 @@ final class Connection {
public String toString() {
return connectionLabel;
}
+
+ /**
+ * Self-cancelling tasks that periodically checks for the pending queue to clear before shutting down the
+ * {@code Connection}. Once it does that, it self cancels the scheduled job in the executor.
+ */
+ private final class CheckForPending implements Runnable {
+ private volatile ScheduledFuture<?> self;
+ private final CompletableFuture<Void> future;
+
+ CheckForPending(final CompletableFuture<Void> future) {
+ this.future = future;
+ }
+
+ @Override
+ public void run() {
+ logger.info("Checking for pending messages to complete before close on {}", this);
+ if (pending.isEmpty()) {
+ shutdown(future);
+ boolean interrupted = false;
+ try {
+ while(null == self) {
+ try {
+ Thread.sleep(1);
+ } catch (InterruptedException e) {
+ interrupted = true;
+ }
+ }
+ self.cancel(false);
+ } finally {
+ if(interrupted) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ }
+ }
+
+ void runUntilDone(final ScheduledExecutorService executor, final long period, final TimeUnit unit) {
+ self = executor.scheduleAtFixedRate(this, period, period, unit);
+ }
+ }
}
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
index 9955e82..4691b1b 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ConnectionPool.java
@@ -219,27 +219,26 @@ final class ConnectionPool {
/**
* Permanently kills the pool.
*/
- public CompletableFuture<Void> closeAsync() {
- logger.info("Signalled closing of connection pool on {} with core size of {}", host, minPoolSize);
+ public synchronized CompletableFuture<Void> closeAsync() {
+ if (closeFuture.get() != null) return closeFuture.get();
- CompletableFuture<Void> future = closeFuture.get();
- if (future != null)
- return future;
+ logger.info("Signalled closing of connection pool on {} with core size of {}", host, minPoolSize);
announceAllAvailableConnection();
- future = CompletableFuture.allOf(killAvailableConnections());
-
- return closeFuture.compareAndSet(null, future) ? future : closeFuture.get();
+ final CompletableFuture<Void> future = killAvailableConnections();
+ closeFuture.set(future);
+ return future;
}
- private CompletableFuture[] killAvailableConnections() {
+ private CompletableFuture<Void> killAvailableConnections() {
final List<CompletableFuture<Void>> futures = new ArrayList<>(connections.size());
for (Connection connection : connections) {
final CompletableFuture<Void> future = connection.closeAsync();
- future.thenRunAsync(open::decrementAndGet, cluster.executor());
+ future.thenRun(open::decrementAndGet);
futures.add(future);
}
- return futures.toArray(new CompletableFuture[futures.size()]);
+
+ return CompletableFuture.allOf(futures.toArray(new CompletableFuture[futures.size()]));
}
void replaceConnection(final Connection connection) {
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java
index 773322a..65eb662 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Handler.java
@@ -29,11 +29,11 @@ import io.netty.util.Attribute;
import io.netty.util.AttributeKey;
import io.netty.util.ReferenceCountUtil;
import org.apache.tinkerpop.gremlin.driver.ser.SerializationException;
+import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.net.InetSocketAddress;
-import java.nio.charset.StandardCharsets;
import java.security.PrivilegedExceptionAction;
import java.security.PrivilegedActionException;
import java.util.HashMap;
@@ -83,11 +83,24 @@ final class Handler {
if (response.getStatus().getCode() == ResponseStatusCode.AUTHENTICATE) {
final Attribute<SaslClient> saslClient = channelHandlerContext.attr(saslClientKey);
final Attribute<Subject> subject = channelHandlerContext.attr(subjectKey);
- RequestMessage.Builder messageBuilder = RequestMessage.build(Tokens.OPS_AUTHENTICATION);
+ final RequestMessage.Builder messageBuilder = RequestMessage.build(Tokens.OPS_AUTHENTICATION);
// First time through we don't have a sasl client
if (saslClient.get() == null) {
subject.set(login());
- saslClient.set(saslClient(getHostName(channelHandlerContext)));
+ try {
+ saslClient.set(saslClient(getHostName(channelHandlerContext)));
+ } catch (SaslException saslException) {
+ // push the sasl error into a failure response from the server. this ensures that standard
+ // processing for the ResultQueue is kept. without this SaslException trap and subsequent
+ // conversion to an authentication failure, the close() of the connection might not
+ // succeed as it will appear as though pending messages remain present in the queue on the
+ // connection and the shutdown won't proceed
+ final ResponseMessage clientSideError = ResponseMessage.build(response.getRequestId())
+ .code(ResponseStatusCode.FORBIDDEN).statusMessage(saslException.getMessage()).create();
+ channelHandlerContext.fireChannelRead(clientSideError);
+ return;
+ }
+
messageBuilder.addArg(Tokens.ARGS_SASL_MECHANISM, getMechanism());
messageBuilder.addArg(Tokens.ARGS_SASL, saslClient.get().hasInitialResponse() ?
evaluateChallenge(subject, saslClient, NULL_CHALLENGE) : null);
@@ -214,12 +227,12 @@ final class Handler {
// there are that many failures someone would take notice and hopefully stop the client.
logger.error("Could not process the response", cause);
- // the channel took an error because of something pretty bad so release all the completeable
- // futures out there
- pending.entrySet().stream().forEach(kv -> kv.getValue().markError(cause));
+ // the channel took an error because of something pretty bad so release all the futures out there
+ pending.values().forEach(val -> val.markError(cause));
+ pending.clear();
// serialization exceptions should not close the channel - that's worth a retry
- if (!ExceptionUtils.getThrowableList(cause).stream().anyMatch(t -> t instanceof SerializationException))
+ if (!IteratorUtils.anyMatch(ExceptionUtils.getThrowableList(cause).iterator(), t -> t instanceof SerializationException))
ctx.close();
}
}
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java
index 89a0225..e55456e 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/ResultQueue.java
@@ -76,6 +76,10 @@ final class ResultQueue {
return this.size() == 0;
}
+ public boolean isComplete() {
+ return readComplete.isDone();
+ }
+
void drainTo(final Collection<Result> collection) {
if (error.get() != null) throw new RuntimeException(error.get());
resultLinkedBlockingQueue.drainTo(collection);
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/handler/WebSocketClientHandler.java
----------------------------------------------------------------------
diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/handler/WebSocketClientHandler.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/handler/WebSocketClientHandler.java
index 922775e..e2a5668 100644
--- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/handler/WebSocketClientHandler.java
+++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/handler/WebSocketClientHandler.java
@@ -95,9 +95,9 @@ public final class WebSocketClientHandler extends SimpleChannelInboundHandler<Ob
@Override
public void exceptionCaught(final ChannelHandlerContext ctx, final Throwable cause) throws Exception {
- logger.warn("Exception caught during WebSocket processing - closing connection", cause);
if (!handshakeFuture.isDone()) handshakeFuture.setFailure(cause);
- ctx.close();
+
+ // let the GremlinResponseHandler take care of exception logging, channel closing, and cleanup
ctx.fireExceptionCaught(cause);
}
}
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java
----------------------------------------------------------------------
diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java
index 1a04b6b..8f24de2 100644
--- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java
+++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java
@@ -25,7 +25,6 @@ import org.apache.tinkerpop.gremlin.driver.Client;
import org.apache.tinkerpop.gremlin.driver.Cluster;
import org.apache.tinkerpop.gremlin.driver.Result;
import org.apache.tinkerpop.gremlin.driver.ResultSet;
-import org.apache.tinkerpop.gremlin.driver.exception.ConnectionException;
import org.apache.tinkerpop.gremlin.driver.exception.ResponseException;
import org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode;
import org.apache.tinkerpop.gremlin.driver.ser.JsonBuilderGryoSerializer;
@@ -38,6 +37,7 @@ import org.apache.tinkerpop.gremlin.util.TimeUtil;
import groovy.json.JsonBuilder;
import org.apache.tinkerpop.gremlin.util.function.FunctionUtils;
import org.hamcrest.core.IsInstanceOf;
+import org.junit.Before;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -746,11 +746,13 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration
client.close();
try {
- client.submit("x[0]+1");
+ client.submit("x[0]+1").all().get();
fail("Should have thrown an exception because the connection is closed");
} catch (Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
- assertThat(root, instanceOf(ConnectionException.class));
+ assertThat(root, instanceOf(IllegalStateException.class));
+ } finally {
+ cluster.close();
}
}
@@ -1248,6 +1250,73 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration
}
}
+ @Test
+ public void shouldCloseAllClientsOnCloseOfCluster() throws Exception {
+ final Cluster cluster = Cluster.open();
+ final Client sessionlessOne = cluster.connect();
+ final Client session = cluster.connect("session");
+ final Client sessionlessTwo = cluster.connect();
+ final Client sessionlessThree = cluster.connect();
+ final Client sessionlessFour = cluster.connect();
+
+ assertEquals(2, sessionlessOne.submit("1+1").all().get().get(0).getInt());
+ assertEquals(2, session.submit("1+1").all().get().get(0).getInt());
+ assertEquals(2, sessionlessTwo.submit("1+1").all().get().get(0).getInt());
+ assertEquals(2, sessionlessThree.submit("1+1").all().get().get(0).getInt());
+ // dont' send anything on the 4th client
+
+ // close one of these Clients before the Cluster
+ sessionlessThree.close();
+ cluster.close();
+
+ try {
+ sessionlessOne.submit("1+1").all().get();
+ } catch (Exception ex) {
+ final Throwable root = ExceptionUtils.getRootCause(ex);
+ assertThat(root, instanceOf(IllegalStateException.class));
+ assertEquals("Client has been closed", root.getMessage());
+ }
+
+ try {
+ session.submit("1+1").all().get();
+ } catch (Exception ex) {
+ final Throwable root = ExceptionUtils.getRootCause(ex);
+ assertThat(root, instanceOf(IllegalStateException.class));
+ assertEquals("Client has been closed", root.getMessage());
+ }
+
+ try {
+ sessionlessTwo.submit("1+1").all().get();
+ } catch (Exception ex) {
+ final Throwable root = ExceptionUtils.getRootCause(ex);
+ assertThat(root, instanceOf(IllegalStateException.class));
+ assertEquals("Client has been closed", root.getMessage());
+ }
+
+ try {
+ sessionlessThree.submit("1+1").all().get();
+ } catch (Exception ex) {
+ final Throwable root = ExceptionUtils.getRootCause(ex);
+ assertThat(root, instanceOf(IllegalStateException.class));
+ assertEquals("Client has been closed", root.getMessage());
+ }
+
+ try {
+ sessionlessFour.submit("1+1").all().get();
+ } catch (Exception ex) {
+ final Throwable root = ExceptionUtils.getRootCause(ex);
+ assertThat(root, instanceOf(IllegalStateException.class));
+ assertEquals("Client has been closed", root.getMessage());
+ }
+
+ // allow call to close() even though closed through cluster
+ sessionlessOne.close();
+ session.close();
+ sessionlessTwo.close();
+
+ cluster.close();
+ }
+
private void assertFutureTimeout(final CompletableFuture<List<Result>> futureFirst) {
try
{
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java
----------------------------------------------------------------------
diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java
index 3e1b7e9..887d408 100644
--- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java
+++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthIntegrateTest.java
@@ -23,7 +23,6 @@ import org.apache.tinkerpop.gremlin.driver.Client;
import org.apache.tinkerpop.gremlin.driver.Cluster;
import org.apache.tinkerpop.gremlin.driver.exception.ResponseException;
import org.apache.tinkerpop.gremlin.server.auth.SimpleAuthenticator;
-import org.ietf.jgss.GSSException;
import org.junit.Test;
import java.util.HashMap;
@@ -128,7 +127,7 @@ public class GremlinServerAuthIntegrateTest extends AbstractGremlinServerIntegra
fail("This should not succeed as the client did not provide credentials");
} catch(Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
- assertEquals(GSSException.class, root.getClass());
+ assertEquals(ResponseException.class, root.getClass());
} finally {
cluster.close();
}
@@ -157,7 +156,7 @@ public class GremlinServerAuthIntegrateTest extends AbstractGremlinServerIntegra
final Client client = cluster.connect();
try {
- client.submit("1+1").all();
+ client.submit("1+1").all().get();
} catch(Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
assertEquals(ResponseException.class, root.getClass());
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthOldIntegrateTest.java
----------------------------------------------------------------------
diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthOldIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthOldIntegrateTest.java
index f2e5622..2f332be 100644
--- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthOldIntegrateTest.java
+++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerAuthOldIntegrateTest.java
@@ -131,7 +131,7 @@ public class GremlinServerAuthOldIntegrateTest extends AbstractGremlinServerInte
fail("This should not succeed as the client did not provide credentials");
} catch(Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
- assertEquals(GSSException.class, root.getClass());
+ assertEquals(ResponseException.class, root.getClass());
// removed this assert as the text of the message changes based on kerberos config - stupid kerberos
// assertThat(root.getMessage(), startsWith("Invalid name provided"));
@@ -163,7 +163,7 @@ public class GremlinServerAuthOldIntegrateTest extends AbstractGremlinServerInte
final Client client = cluster.connect();
try {
- client.submit("1+1").all();
+ client.submit("1+1").all().get();
} catch(Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
assertEquals(ResponseException.class, root.getClass());
http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/c3acce6e/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSessionIntegrateTest.java
----------------------------------------------------------------------
diff --git a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSessionIntegrateTest.java b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSessionIntegrateTest.java
index 99b3a1b..3c1fef9 100644
--- a/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSessionIntegrateTest.java
+++ b/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerSessionIntegrateTest.java
@@ -26,7 +26,6 @@ import org.apache.tinkerpop.gremlin.driver.Cluster;
import org.apache.tinkerpop.gremlin.driver.Result;
import org.apache.tinkerpop.gremlin.driver.ResultSet;
import org.apache.tinkerpop.gremlin.driver.Tokens;
-import org.apache.tinkerpop.gremlin.driver.exception.ConnectionException;
import org.apache.tinkerpop.gremlin.driver.exception.ResponseException;
import org.apache.tinkerpop.gremlin.driver.message.RequestMessage;
import org.apache.tinkerpop.gremlin.driver.message.ResponseMessage;
@@ -189,7 +188,7 @@ public class GremlinServerSessionIntegrateTest extends AbstractGremlinServerInt
fail("Session should be dead");
} catch (Exception ex) {
final Throwable root = ExceptionUtils.getRootCause(ex);
- assertThat(root, instanceOf(ConnectionException.class));
+ assertThat(root, instanceOf(IllegalStateException.class));
} finally {
cluster.close();
}
@@ -229,7 +228,8 @@ public class GremlinServerSessionIntegrateTest extends AbstractGremlinServerInt
cluster.close();
}
- assertEquals(1, recordingAppender.getMessages().stream()
+ // there will be on for the timeout and a second for closing the cluster
+ assertEquals(2, recordingAppender.getMessages().stream()
.filter(msg -> msg.equals("INFO - Session shouldHaveTheSessionTimeout closed\n")).count());
}