TINKERPOP-1350 was never quite fixed in 3.1.3. Changed response encoding to not use the session executor when the session has an error condition it is trying to serialize. This should be fine as there is no need to serialized an error condition as part of a transaction and thus no need to have the session thread to do it. That in turn frees up the worker executor to serialize and cancel long run jobs in the session. Removed recommendations for submitting parallel requests on a session from docs.
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/58d8bade Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/58d8bade Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/58d8bade Branch: refs/heads/TINKERPOP-1350 Commit: 58d8bade7425c7a7865382990eaaed2b7d90659c Parents: 87960e7 Author: Stephen Mallette <sp...@genoprime.com> Authored: Tue Aug 2 15:59:19 2016 -0400 Committer: Stephen Mallette <sp...@genoprime.com> Committed: Mon Aug 8 06:49:17 2016 -0400 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../src/reference/gremlin-applications.asciidoc | 6 +- .../upgrade/release-3.1.x-incubating.asciidoc | 22 +++++- .../gremlin/groovy/engine/GremlinExecutor.java | 2 - .../handler/GremlinResponseFrameEncoder.java | 13 +++- .../server/GremlinDriverIntegrateTest.java | 81 +++++++++++--------- 6 files changed, 75 insertions(+), 50 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/58d8bade/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 306185c..975f6cf 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -28,6 +28,7 @@ TinkerPop 3.1.4 (NOT OFFICIALLY RELEASED YET) * Fixed a potential leak of a `ReferenceCounted` resource in Gremlin Server. * Renamed distributions to make the prefix "apache-tinkerpop-" as opposed to just "apache-". +* Fixed a problem (previously thought resolved on 3.1.3) causing Gremlin Server to lock up when parallel requests were submitted on the same session if those parallel requests included a script that blocked indefinitely. [[release-3-1-3]] TinkerPop 3.1.3 (Release Date: July 18, 2016) http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/58d8bade/docs/src/reference/gremlin-applications.asciidoc ---------------------------------------------------------------------- diff --git a/docs/src/reference/gremlin-applications.asciidoc b/docs/src/reference/gremlin-applications.asciidoc index 9f5121f..cedd98f 100644 --- a/docs/src/reference/gremlin-applications.asciidoc +++ b/docs/src/reference/gremlin-applications.asciidoc @@ -1261,7 +1261,7 @@ Tuning image:gremlin-handdrawn.png[width=120,float=right] Tuning Gremlin Server for a particular environment may require some simple trial-and-error, but the following represent some basic guidelines that might be useful: * Gremlin Server defaults to a very modest maximum heap size. Consider increasing this value for non-trivial uses. Maximum heap size (`-Xmx`) is defined with the `JAVA_OPTIONS` setting in `gremlin-server.sh`. -* When configuring the size of `threadPoolWorker` start with the default of `1` and increment by one as needed to a maximum of `2*number of cores`. Note that if using sessions that will accept parallel requests on the same session, then this value should be no less than `2`. +* When configuring the size of `threadPoolWorker` start with the default of `1` and increment by one as needed to a maximum of `2*number of cores`. * The "right" size of the `gremlinPool` setting is somewhat dependent on the type of scripts that will be processed by Gremlin Server. As requests arrive to Gremlin Server they are decoded and queued to be processed by threads in this pool. When this pool is exhausted of threads, Gremlin Server will continue to accept incoming requests, but @@ -1407,10 +1407,6 @@ request. A session is a "heavier" approach to the simple "request/response" approach of sessionless requests, but is sometimes necessary for a given use case. -IMPORTANT: If submitting requests in parallel to a single session in Gremlin Server, then the `threadPoolWorker` -setting can be no less than `2` or else the session may be prone to becoming locked if scripts sent on that session -tend to block for extended periods of time. - [[considering-transactions]] Considering Transactions ^^^^^^^^^^^^^^^^^^^^^^^^ http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/58d8bade/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 11cdb32..b4a1657 100644 --- a/docs/src/upgrade/release-3.1.x-incubating.asciidoc +++ b/docs/src/upgrade/release-3.1.x-incubating.asciidoc @@ -22,6 +22,26 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima *A 187 On The Undercover Gremlinz* +TinkerPop 3.1.4 +--------------- + +*Release Date: NOT OFFICIALLY RELEASED YET* + +Please see the link:https://github.com/apache/tinkerpop/blob/3.1.4/CHANGELOG.asciidoc#tinkerpop-314-release-date-XXXXXXX-XX-2016[changelog] for a complete list of all the modifications that are part of this release. + +Upgrading for Users +~~~~~~~~~~~~~~~~~~~ + +Gremlin Server Workers +^^^^^^^^^^^^^^^^^^^^^^ + +In release 3.1.3, a link:http://tinkerpop.apache.org/docs/3.1.3/upgrade/#_tinkerpop_3_1_3[recommendation] was made to +ensure that the `threadPoolWorker` setting for Gremlin Server was no less than `2` in cases where Gremlin Server was +being used with sessions that accept parallel requests. In 3.1.4, that is no longer the case and a size of `1` remains +acceptable even in that specific case. + +See: link:https://issues.apache.org/jira/browse/TINKERPOP-1350[TINKERPOP-1350] + TinkerPop 3.1.3 --------------- @@ -74,8 +94,6 @@ those that block for an extended period of time) may cause Gremlin Server to loc See: link:https://issues.apache.org/jira/browse/TINKERPOP-1350[TINKERPOP-1350] - - Upgrading for Providers ~~~~~~~~~~~~~~~~~~~~~~~ http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/58d8bade/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java ---------------------------------------------------------------------- diff --git a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java index d70ff3d..da12e1e 100644 --- a/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java +++ b/gremlin-groovy/src/main/java/org/apache/tinkerpop/gremlin/groovy/engine/GremlinExecutor.java @@ -232,8 +232,6 @@ public class GremlinExecutor implements AutoCloseable { return eval(script, language, boundVars, lifeCycle); } - private static final AtomicInteger ugh = new AtomicInteger(0); - /** * Evaluate a script and allow for the submission of alteration to the entire evaluation execution lifecycle. * http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/58d8bade/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/GremlinResponseFrameEncoder.java ---------------------------------------------------------------------- diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/GremlinResponseFrameEncoder.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/GremlinResponseFrameEncoder.java index 50c177b..d0f9d76 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/GremlinResponseFrameEncoder.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/GremlinResponseFrameEncoder.java @@ -61,8 +61,12 @@ public class GremlinResponseFrameEncoder extends MessageToMessageEncoder<Respons if (useBinary) { final Frame serialized; - // if the request came in on a session then the serialization must occur in that same thread. - if (null == session) + // if the request came in on a session then the serialization must occur in that same thread, except + // in the case of an error where we can free the session executor from having to do that job. the + // problem here is that if the session executor is used in the case of an error and the executor is + // blocked by parallel requests then there is no thread available to serialize the result and send + // back the response as the workers get all tied up behind the session executor. + if (null == session || !o.getStatus().getCode().isSuccess()) serialized = new Frame(serializer.serializeResponseAsBinary(o, ctx.alloc())); else serialized = new Frame(session.getExecutor().submit(() -> serializer.serializeResponseAsBinary(o, ctx.alloc())).get()); @@ -75,8 +79,9 @@ public class GremlinResponseFrameEncoder extends MessageToMessageEncoder<Respons final Frame serialized; - // if the request came in on a session then the serialization must occur in that same thread. - if (null == session) + // if the request came in on a session then the serialization must occur that same thread except + // in the case of errors for reasons described above. + if (null == session || !o.getStatus().getCode().isSuccess()) serialized = new Frame(textSerializer.serializeResponseAsString(o)); else serialized = new Frame(session.getExecutor().submit(() -> textSerializer.serializeResponseAsString(o)).get()); http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/58d8bade/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 7f74e03..7314243 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 @@ -125,8 +125,8 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration settings.graphs.put("graph", "conf/neo4j-empty.properties"); break; case "shouldProcessSessionRequestsInOrderAfterTimeout": - settings.scriptEvaluationTimeout = 1000; - settings.threadPoolWorker = 2; + settings.scriptEvaluationTimeout = 250; + settings.threadPoolWorker = 1; break; } @@ -1210,48 +1210,55 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration final Cluster cluster = Cluster.open(); final Client client = cluster.connect(name.getMethodName()); - final ResultSet first = client.submit( - "Object mon1 = 'mon1';\n" + - "synchronized (mon1) {\n" + - " mon1.wait();\n" + - "} "); - - final ResultSet second = client.submit( - "Object mon2 = 'mon2';\n" + - "synchronized (mon2) {\n" + - " mon2.wait();\n" + - "}"); - - final CompletableFuture<List<Result>> futureFirst = first.all(); - final CompletableFuture<List<Result>> futureSecond = second.all(); - - final AtomicBoolean hit = new AtomicBoolean(false); - while (!futureFirst.isDone()) { - // futureSecond can't finish before futureFirst - racy business here? - assertThat(futureSecond.isDone(), is(false)); - hit.set(true); + for(int index = 0; index < 50; index++) + { + final CompletableFuture<ResultSet> first = client.submitAsync( + "Object mon1 = 'mon1';\n" + + "synchronized (mon1) {\n" + + " mon1.wait();\n" + + "} "); + + final CompletableFuture<ResultSet> second = client.submitAsync( + "Object mon2 = 'mon2';\n" + + "synchronized (mon2) {\n" + + " mon2.wait();\n" + + "}"); + + final CompletableFuture<ResultSet> third = client.submitAsync( + "Object mon3 = 'mon3';\n" + + "synchronized (mon3) {\n" + + " mon3.wait();\n" + + "}"); + + final CompletableFuture<ResultSet> fourth = client.submitAsync( + "Object mon4 = 'mon4';\n" + + "synchronized (mon4) {\n" + + " mon4.wait();\n" + + "}"); + + final CompletableFuture<List<Result>> futureFirst = first.get().all(); + final CompletableFuture<List<Result>> futureSecond = second.get().all(); + final CompletableFuture<List<Result>> futureThird = third.get().all(); + final CompletableFuture<List<Result>> futureFourth = fourth.get().all(); + + assertFutureTimeout(futureFirst); + assertFutureTimeout(futureSecond); + assertFutureTimeout(futureThird); + assertFutureTimeout(futureFourth); } + } - // should have entered the loop at least once and thus proven that futureSecond didn't return ahead of - // futureFirst - assertThat(hit.get(), is(true)); - - try { + private void assertFutureTimeout(final CompletableFuture<List<Result>> futureFirst) { + try + { futureFirst.get(); fail("Should have timed out"); - } catch (Exception ex) { - final Throwable root = ExceptionUtils.getRootCause(ex); - assertThat(root, instanceOf(ResponseException.class)); - assertThat(root.getMessage(), startsWith("Script evaluation exceeded the configured 'scriptEvaluationTimeout' threshold of 1000 ms for request")); } - - try { - futureSecond.get(); - fail("Should have timed out"); - } catch (Exception ex) { + catch (Exception ex) + { final Throwable root = ExceptionUtils.getRootCause(ex); assertThat(root, instanceOf(ResponseException.class)); - assertThat(root.getMessage(), startsWith("Script evaluation exceeded the configured 'scriptEvaluationTimeout' threshold of 1000 ms for request")); + assertThat(root.getMessage(), startsWith("Script evaluation exceeded the configured 'scriptEvaluationTimeout' threshold of 250 ms for request")); } } }
