Simplified Gremlin Server error handling CTR
Project: http://git-wip-us.apache.org/repos/asf/tinkerpop/repo Commit: http://git-wip-us.apache.org/repos/asf/tinkerpop/commit/a7e0e0f5 Tree: http://git-wip-us.apache.org/repos/asf/tinkerpop/tree/a7e0e0f5 Diff: http://git-wip-us.apache.org/repos/asf/tinkerpop/diff/a7e0e0f5 Branch: refs/heads/TINKERPOP-1682 Commit: a7e0e0f5008b20e2d16ecfa2e4890468ef2a53a7 Parents: 3f06a6a Author: Stephen Mallette <[email protected]> Authored: Sun Jul 23 18:47:54 2017 -0400 Committer: Stephen Mallette <[email protected]> Committed: Sun Jul 23 18:47:54 2017 -0400 ---------------------------------------------------------------------- CHANGELOG.asciidoc | 1 + .../gremlin/groovy/engine/GremlinExecutor.java | 1 - .../server/op/AbstractEvalOpProcessor.java | 29 +++----------- .../server/GremlinDriverIntegrateTest.java | 40 +++++++++++++++++++- 4 files changed, 44 insertions(+), 27 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/a7e0e0f5/CHANGELOG.asciidoc ---------------------------------------------------------------------- diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 3a0627d..9039d9f 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -28,6 +28,7 @@ TinkerPop 3.2.6 (Release Date: NOT OFFICIALLY RELEASED YET) This release also includes changes from <<release-3-1-8, 3.1.8>>. +* Exceptions that occur during result iteration in Gremlin Server will now return `SCRIPT_EVALUATION_EXCEPTION` rather than `SERVER_ERROR`. * Initialization scripts for Gremlin Server will not timeout. * Added Gremlin.Net. * `ProfileTest` is now less stringent about assertions which will reduce burdens on providers. http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/a7e0e0f5/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 962f798..fabc8cb 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 @@ -331,7 +331,6 @@ public class GremlinExecutor implements AutoCloseable { // Schedule a timeout in the thread pool for future execution scheduledExecutorService.schedule(() -> { if (executionFuture.cancel(true)) { - lifeCycle.getAfterTimeout().orElse(afterTimeout).accept(bindings); final CompletableFuture<Object> ef = evaluationFutureRef.get(); if (ef != null) { ef.completeExceptionally(new TimeoutException( http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/a7e0e0f5/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java ---------------------------------------------------------------------- diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java index a76323f..46b3c8d 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/op/AbstractEvalOpProcessor.java @@ -266,31 +266,12 @@ public abstract class AbstractEvalOpProcessor extends AbstractOpProcessor { try { handleIterator(context, itty); - } catch (TimeoutException ex) { - // a timeout occurs if serializedResponseTimeout is exceeded, but that setting is deprecated - // and by default disabled. we can remove this handling once we drop that setting all together - final String errorMessage = String.format("Response iteration exceeded the configured threshold for request [%s] - %s", msg, ex.getMessage()); - logger.warn(errorMessage); - ctx.writeAndFlush(ResponseMessage.build(msg).code(ResponseStatusCode.SERVER_ERROR_TIMEOUT) - .statusMessage(errorMessage) - .statusAttributeException(ex).create()); - if (managedTransactionsForRequest) attemptRollback(msg, context.getGraphManager(), settings.strictTransactionManagement); - } catch (InterruptedException ex) { - // interruption occurs if there is a forced timeout during result iteration. this timeout - // is driven by the script evaluation timeout so for consistency the message should be the same - final String errorMessage = String.format("Script evaluation exceeded the configured threshold for request [%s] - %s", msg, ex.getMessage()); - logger.warn(errorMessage, ex); - ctx.writeAndFlush(ResponseMessage.build(msg).code(ResponseStatusCode.SERVER_ERROR_TIMEOUT) - .statusMessage(ex.getMessage()) - .statusAttributeException(ex).create()); - if (managedTransactionsForRequest) attemptRollback(msg, context.getGraphManager(), settings.strictTransactionManagement); } catch (Exception ex) { - logger.warn(String.format("Exception processing a script on request [%s].", msg), ex); - final String err = ex.getMessage(); - ctx.writeAndFlush(ResponseMessage.build(msg).code(ResponseStatusCode.SERVER_ERROR) - .statusMessage(null == err || err.isEmpty() ? ex.getClass().getSimpleName() : err) - .statusAttributeException(ex).create()); if (managedTransactionsForRequest) attemptRollback(msg, context.getGraphManager(), settings.strictTransactionManagement); + + // wrap up the exception and rethrow. the error will be written to the client by the evalFuture + // as it will completeExceptionally in the GremlinExecutor + throw new RuntimeException(ex); } }).create(); @@ -317,7 +298,7 @@ public abstract class AbstractEvalOpProcessor extends AbstractOpProcessor { .statusMessage("Timeout during script evaluation triggered by TimedInterruptCustomizerProvider") .statusAttributeException(t).create()); } else if (t instanceof TimeoutException) { - final String errorMessage = String.format("Script evaluation exceeded the configured threshold for request [%s] - %s", msg, t.getMessage()); + final String errorMessage = String.format("Script evaluation exceeded the configured threshold for request [%s]", msg); logger.warn(errorMessage, t); ctx.writeAndFlush(ResponseMessage.build(msg).code(ResponseStatusCode.SERVER_ERROR_TIMEOUT) .statusMessage(t.getMessage()) http://git-wip-us.apache.org/repos/asf/tinkerpop/blob/a7e0e0f5/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 6d4f236..9662ab7 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 @@ -171,12 +171,48 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration settings.scriptEvaluationTimeout = 250; settings.threadPoolWorker = 1; break; + case "shouldProcessTraversalInterruption": + case "shouldProcessEvalInterruption": + settings.scriptEvaluationTimeout = 1500; + break; } return settings; } @Test + public void shouldProcessTraversalInterruption() throws Exception { + final Cluster cluster = TestClientFactory.open(); + final Client client = cluster.connect(); + + try { + client.submit("g.inject(1).sideEffect{Thread.sleep(5000)}").all().get(); + fail("Should have timed out"); + } catch (Exception ex) { + final ResponseException re = (ResponseException) ex.getCause(); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, re.getResponseStatusCode()); + } + + cluster.close(); + } + + @Test + public void shouldProcessEvalInterruption() throws Exception { + final Cluster cluster = TestClientFactory.open(); + final Client client = cluster.connect(); + + try { + client.submit("Thread.sleep(5000);'done'").all().get(); + fail("Should have timed out"); + } catch (Exception ex) { + final ResponseException re = (ResponseException) ex.getCause(); + assertEquals(ResponseStatusCode.SERVER_ERROR_TIMEOUT, re.getResponseStatusCode()); + } + + cluster.close(); + } + + @Test public void shouldKeepAliveForWebSockets() throws Exception { // keep the connection pool size at 1 to remove the possibility of lots of connections trying to ping which will // complicate the assertion logic @@ -1189,7 +1225,7 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration final Throwable root = ExceptionUtils.getRootCause(ex); assertThat(root, instanceOf(ResponseException.class)); final ResponseException re = (ResponseException) root; - assertEquals(ResponseStatusCode.SERVER_ERROR, re.getResponseStatusCode()); + assertEquals(ResponseStatusCode.SERVER_ERROR_SCRIPT_EVALUATION, re.getResponseStatusCode()); } // keep the testing here until "rebind" is completely removed @@ -1245,7 +1281,7 @@ public class GremlinDriverIntegrateTest extends AbstractGremlinServerIntegration final Throwable root = ExceptionUtils.getRootCause(ex); assertThat(root, instanceOf(ResponseException.class)); final ResponseException re = (ResponseException) root; - assertEquals(ResponseStatusCode.SERVER_ERROR, re.getResponseStatusCode()); + assertEquals(ResponseStatusCode.SERVER_ERROR_SCRIPT_EVALUATION, re.getResponseStatusCode()); } // keep the testing here until "rebind" is completely removed
