From b29bab0acada54aa525184935d96b41f9cf0854b Mon Sep 17 00:00:00 2001 From: Stephen Mallette Date: Thu, 6 Apr 2017 17:55:39 -0400 Subject: [PATCH 1/2] TINKERPOP-1044 Additional error data to Gremlin Server responses Applies to all protocols: http, nio, and websocket. Added stack trace and exception hierarchy. Deprecated the "Exception-Class" in the http protocol. --- .../tinkerpop/gremlin/driver/Handler.java | 11 +++++- .../tinkerpop/gremlin/driver/Tokens.java | 3 ++ .../driver/exception/ResponseException.java | 20 ++++++++++ .../driver/message/ResponseMessage.java | 19 ++++++++++ .../gremlin/driver/ResultQueueTest.java | 2 - .../handler/GremlinResponseFrameEncoder.java | 6 ++- .../handler/HttpGremlinEndpointHandler.java | 9 ++++- .../server/handler/IteratorHandler.java | 5 ++- .../handler/NioGremlinResponseEncoder.java | 5 ++- .../server/handler/OpExecutorHandler.java | 1 + .../server/handler/OpSelectorHandler.java | 4 +- .../handler/WsGremlinResponseEncoder.java | 5 ++- .../server/op/AbstractEvalOpProcessor.java | 31 +++++++++++---- .../server/op/AbstractOpProcessor.java | 5 ++- .../op/traversal/TraversalOpProcessor.java | 35 ++++++++++++----- .../gremlin/server/util/ExceptionHelper.java | 38 +++++++++++++++++++ .../server/GremlinDriverIntegrateTest.java | 5 +++ .../GremlinServerHttpIntegrateTest.java | 8 ++++ 18 files changed, 179 insertions(+), 33 deletions(-) create mode 100644 gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/util/ExceptionHelper.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 650bc2f752b..1bd0a3b5a45 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 @@ -235,8 +235,15 @@ protected void channelRead0(final ChannelHandlerContext channelHandlerContext, f } } else { // this is a "success" but represents no results otherwise it is an error - if (statusCode != ResponseStatusCode.NO_CONTENT) - queue.markError(new ResponseException(response.getStatus().getCode(), response.getStatus().getMessage())); + if (statusCode != ResponseStatusCode.NO_CONTENT) { + final Map attributes = response.getStatus().getAttributes(); + final String stackTrace = attributes.containsKey(Tokens.STATUS_ATTRIBUTE_STACK_TRACE) ? + (String) attributes.get(Tokens.STATUS_ATTRIBUTE_STACK_TRACE) : null; + final List exceptions = attributes.containsKey(Tokens.STATUS_ATTRIBUTE_EXCEPTIONS) ? + (List) attributes.get(Tokens.STATUS_ATTRIBUTE_EXCEPTIONS) : null; + queue.markError(new ResponseException(response.getStatus().getCode(), response.getStatus().getMessage(), + exceptions, stackTrace)); + } } // as this is a non-PARTIAL_CONTENT code - the stream is done diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Tokens.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Tokens.java index fb577d71031..9aee728f497 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Tokens.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Tokens.java @@ -155,4 +155,7 @@ private Tokens() {} public static final String VAL_AGGREGATE_TO_SET = "set"; public static final String VAL_TRAVERSAL_SOURCE_ALIAS = "g"; + + public static final String STATUS_ATTRIBUTE_EXCEPTIONS = "exceptions"; + public static final String STATUS_ATTRIBUTE_STACK_TRACE = "stackTrace"; } diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java index 3ae4e75541a..800c2fcc15d 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java @@ -20,18 +20,38 @@ import org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode; +import java.util.List; +import java.util.Optional; + /** * @author Stephen Mallette (http://stephen.genoprime.com) */ public class ResponseException extends Exception { private ResponseStatusCode responseStatusCode; + private String remoteStackTrace = null; + private List remoteExceptionHierarchy = null; public ResponseException(final ResponseStatusCode responseStatusCode, final String serverMessage) { super(serverMessage); this.responseStatusCode = responseStatusCode; } + public ResponseException(final ResponseStatusCode responseStatusCode, final String serverMessage, + final List remoteExceptionHierarchy, final String remoteStackTrace) { + this(responseStatusCode, serverMessage); + this.remoteExceptionHierarchy = remoteExceptionHierarchy; + this.remoteStackTrace = remoteStackTrace; + } + public ResponseStatusCode getResponseStatusCode() { return responseStatusCode; } + + public Optional getRemoteStackTrace() { + return Optional.ofNullable(remoteStackTrace); + } + + public Optional> getRemoteExceptionHierarchy() { + return Optional.ofNullable(remoteExceptionHierarchy); + } } \ No newline at end of file diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/message/ResponseMessage.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/message/ResponseMessage.java index c5a00f6281c..f4126b1d5d0 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/message/ResponseMessage.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/message/ResponseMessage.java @@ -18,7 +18,12 @@ */ package org.apache.tinkerpop.gremlin.driver.message; +import org.apache.commons.lang.exception.ExceptionUtils; +import org.apache.tinkerpop.gremlin.driver.Tokens; +import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; + import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.UUID; @@ -102,6 +107,20 @@ public Builder statusAttributes(final Map attributes) { return this; } + public Builder statusAttributeException(final Throwable ex) { + statusAttribute(Tokens.STATUS_ATTRIBUTE_EXCEPTIONS, IteratorUtils.asList( + IteratorUtils.map(ExceptionUtils.getThrowableList(ex), t -> t.getClass().getName()))); + statusAttribute(Tokens.STATUS_ATTRIBUTE_STACK_TRACE, ExceptionUtils.getFullStackTrace(ex)); + return this; + } + + public Builder statusAttribute(final String key, final Object value) { + if (this.attributes == Collections.EMPTY_MAP) + attributes = new HashMap<>(); + attributes.put(key, value); + return this; + } + public Builder result(final Object result) { this.result = result; return this; diff --git a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java index 67bbc482136..a7e60663b98 100644 --- a/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java +++ b/gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/ResultQueueTest.java @@ -37,8 +37,6 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.greaterThan; -import static org.hamcrest.core.IsCollectionContaining.hasItem; -import static org.hamcrest.Matchers.contains; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; 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 d0f9d76a5c1..3aa8c36779a 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 @@ -20,11 +20,13 @@ import com.codahale.metrics.Meter; import org.apache.tinkerpop.gremlin.driver.MessageSerializer; +import org.apache.tinkerpop.gremlin.driver.Tokens; import org.apache.tinkerpop.gremlin.driver.message.ResponseMessage; import org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode; import org.apache.tinkerpop.gremlin.driver.ser.MessageTextSerializer; import org.apache.tinkerpop.gremlin.server.GremlinServer; import org.apache.tinkerpop.gremlin.server.op.session.Session; +import org.apache.tinkerpop.gremlin.server.util.ExceptionHelper; import org.apache.tinkerpop.gremlin.server.util.MetricManager; import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; @@ -91,10 +93,10 @@ protected void encode(final ChannelHandlerContext ctx, final ResponseMessage o, } catch (Exception ex) { errorMeter.mark(); logger.warn("The result [{}] in the request {} could not be serialized and returned.", o.getResult(), o.getRequestId(), ex); - final String errorMessage = String.format("Error during serialization: %s", - ex.getCause() != null ? ex.getCause().getMessage() : ex.getMessage()); + final String errorMessage = String.format("Error during serialization: %s", ExceptionHelper.getMessageFromExceptionOrCause(ex)); final ResponseMessage error = ResponseMessage.build(o.getRequestId()) .statusMessage(errorMessage) + .statusAttributeException(ex) .code(ResponseStatusCode.SERVER_ERROR_SERIALIZATION).create(); if (useBinary) { objects.add(serializer.serializeResponseAsBinary(error, ctx.alloc())); diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java index 899d488cc60..a1030ccbc33 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpGremlinEndpointHandler.java @@ -20,6 +20,7 @@ import com.codahale.metrics.Meter; import com.codahale.metrics.Timer; +import org.apache.commons.lang.exception.ExceptionUtils; import org.apache.tinkerpop.gremlin.driver.MessageSerializer; import org.apache.tinkerpop.gremlin.driver.Tokens; import org.apache.tinkerpop.gremlin.driver.message.ResponseMessage; @@ -49,7 +50,6 @@ import io.netty.handler.codec.http.QueryStringDecoder; import io.netty.util.CharsetUtil; import io.netty.util.ReferenceCountUtil; -import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.tinkerpop.shaded.jackson.databind.JsonNode; import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper; import org.apache.tinkerpop.shaded.jackson.databind.node.ArrayNode; @@ -457,8 +457,13 @@ private static void sendError(final ChannelHandlerContext ctx, final HttpRespons errorMeter.mark(); final ObjectNode node = mapper.createObjectNode(); node.put("message", message); - if (t.isPresent()){ + if (t.isPresent()) { + // "Exception-Class" needs to go away - didn't realize it was named that way during review for some reason. + // replaced with the same method for exception reporting as is used with websocket/nio protocol node.put("Exception-Class", t.get().getClass().getName()); + final ArrayNode exceptionList = node.putArray(Tokens.STATUS_ATTRIBUTE_EXCEPTIONS); + ExceptionUtils.getThrowableList(t.get()).forEach(throwable -> exceptionList.add(throwable.getClass().getName())); + node.put(Tokens.STATUS_ATTRIBUTE_STACK_TRACE, ExceptionUtils.getFullStackTrace(t.get())); } final FullHttpResponse response = new DefaultFullHttpResponse( diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/IteratorHandler.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/IteratorHandler.java index 527aa37dfdd..78e2dc9cb73 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/IteratorHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/IteratorHandler.java @@ -110,7 +110,10 @@ public void write(final ChannelHandlerContext ctx, final Object msg, final Chann if (!f.isSuccess()) { final String errorMessage = String.format("Response iteration and serialization exceeded the configured threshold for request [%s] - %s", msg, f.cause().getMessage()); logger.warn(errorMessage); - ctx.writeAndFlush(ResponseMessage.build(requestMessage).code(ResponseStatusCode.SERVER_ERROR_TIMEOUT).statusMessage(errorMessage).create()); + ctx.writeAndFlush(ResponseMessage.build(requestMessage) + .code(ResponseStatusCode.SERVER_ERROR_TIMEOUT) + .statusAttributeException(f.cause()) + .statusMessage(errorMessage).create()); } }); } finally { diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/NioGremlinResponseEncoder.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/NioGremlinResponseEncoder.java index 6a98b8b60cd..79368c12953 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/NioGremlinResponseEncoder.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/NioGremlinResponseEncoder.java @@ -25,6 +25,7 @@ import org.apache.tinkerpop.gremlin.driver.message.ResponseStatusCode; import org.apache.tinkerpop.gremlin.driver.ser.MessageTextSerializer; import org.apache.tinkerpop.gremlin.server.GremlinServer; +import org.apache.tinkerpop.gremlin.server.util.ExceptionHelper; import org.apache.tinkerpop.gremlin.server.util.MetricManager; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandler; @@ -72,10 +73,10 @@ protected void encode(final ChannelHandlerContext ctx, final ResponseMessage res } catch (Exception ex) { errorMeter.mark(); logger.warn("The result [{}] in the request {} could not be serialized and returned.", responseMessage.getResult(), responseMessage.getRequestId(), ex); - final String errorMessage = String.format("Error during serialization: %s", - ex.getCause() != null ? ex.getCause().getMessage() : ex.getMessage()); + final String errorMessage = String.format("Error during serialization: %s", ExceptionHelper.getMessageFromExceptionOrCause(ex)); final ResponseMessage error = ResponseMessage.build(responseMessage.getRequestId()) .statusMessage(errorMessage) + .statusAttributeException(ex) .code(ResponseStatusCode.SERVER_ERROR_SERIALIZATION).create(); if (useBinary) { final ByteBuf bytes = serializer.serializeResponseAsBinary(error, ctx.alloc()); diff --git a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/OpExecutorHandler.java b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/OpExecutorHandler.java index eefe600bb34..477cb11fd61 100644 --- a/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/OpExecutorHandler.java +++ b/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/OpExecutorHandler.java @@ -77,6 +77,7 @@ protected void channelRead0(final ChannelHandlerContext ctx, final Pair Date: Thu, 6 Apr 2017 18:10:08 -0400 Subject: [PATCH 2/2] TINKERPOP-1044 Added documentation for additional error info in ResponseMessage --- CHANGELOG.asciidoc | 2 ++ .../upgrade/release-3.2.x-incubating.asciidoc | 35 +++++++++++++++++++ .../driver/exception/ResponseException.java | 7 ++++ 3 files changed, 44 insertions(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 19d1d59bfb9..00ada7e7ef9 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -31,6 +31,8 @@ TinkerPop 3.2.5 (Release Date: NOT OFFICIALLY RELEASED YET) * `ProfileStrategy` now uses the marker-model to reduce recursive lookups of `ProfileSideEffectStep`. * `Mutating` steps now implement `Scoping` interface. * Fixed a step id compilation bug in `AddVertexStartStep`, `AddVertexStep`, `AddEdgeStep`, and `AddPropertyStep`. +* Added more details to Gremlin Server client side messages - exception hierarchy and stack trace. +* Deprecated "Exception-Class" in the Gremlin Server HTTP protocol in favor of the new "exceptions" field. * De-registered metrics on Gremlin Server shutdown. * Added "help" command option on `:remote config` for plugins that support that feature in the Gremlin Console. * Allowed for multiple scripts and related arguments to be passed to `gremlin.sh` via `-i` and `-e`. diff --git a/docs/src/upgrade/release-3.2.x-incubating.asciidoc b/docs/src/upgrade/release-3.2.x-incubating.asciidoc index fbe31bdedea..4324578fbb0 100644 --- a/docs/src/upgrade/release-3.2.x-incubating.asciidoc +++ b/docs/src/upgrade/release-3.2.x-incubating.asciidoc @@ -42,6 +42,41 @@ set. Note that metrics are captured for both sessionless requests as well as for See: https://issues.apache.org/jira/browse/TINKERPOP-1644[TINKERPOP-1644] +Additional Error Information +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Additional information on error responses from Gremlin Server should help make debugging errors easier. Error responses +now have both the exception hierarchy and the stack trace that was generated on the server. In this way, receiving an +error on a client doesn't mean having to rifle through Gremlin Server logs to try to find the associated error. + +This change has been applied to all Gremlin Server protocols. For the binary protocol and the Java driver this change +means that the `ResponseException` thrown from calls to `submit()` requests to the server now have the following +methods: + +[source,java] +---- +public Optional getRemoteStackTrace() + +public Optional> getRemoteExceptionHierarchy() +---- + +The HTTP protocol has also been updated and returns both `exceptions` and `stackTrace` fields in the response: + +[source,js] +---- +{ + "message": "Division by zero", + "Exception-Class": "java.lang.ArithmeticException", + "exceptions": ["java.lang.ArithmeticException"], + "stackTrace": "java.lang.ArithmeticException: Division by zero\n\tat java.math.BigDecimal.divide(BigDecimal.java:1742)\n\tat org.codehaus.groovy.runtime.typehandling.BigDecimalMath.divideImpl(BigDecimalMath.java:68)\n\tat org.codehaus.groovy.runtime.typehandling.IntegerMath.divideImpl(IntegerMath.java:49)\n\tat org.codehaus.groovy.runtime.dgmimpl.NumberNumberDiv$NumberNumber.invoke(NumberNumberDiv.java:323)\n\tat org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:56)\n\tat org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)\n\tat org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)\n\tat org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)\n\tat Script4.run(Script4.groovy:1)\n\tat org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval(GremlinGroovyScriptEngine.java:834)\n\tat org.apache.tinkerpop.gremlin.groovy.jsr223.GremlinGroovyScriptEngine.eval(GremlinGroovyScriptEngine.java:547)\n\tat javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:233)\n\tat org.apache.tinkerpop.gremlin.groovy.engine.ScriptEngines.eval(ScriptEngines.java:120)\n\tat org.apache.tinkerpop.gremlin.groovy.engine.GremlinExecutor.lambda$eval$2(GremlinExecutor.java:314)\n\tat java.util.concurrent.FutureTask.run(FutureTask.java:266)\n\tat java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)\n\tat java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)\n\tat java.lang.Thread.run(Thread.java:745)\n" +} +---- + +Note that the `Exception-Class` which was added in a previous version has been deprecated and replaced by these new +fields. + +See: link:https://issues.apache.org/jira/browse/TINKERPOP-1044[TINKERPOP-1044] + Gremlin Console Scripting ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java index 800c2fcc15d..51e748a6379 100644 --- a/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java +++ b/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/exception/ResponseException.java @@ -47,10 +47,17 @@ public ResponseStatusCode getResponseStatusCode() { return responseStatusCode; } + /** + * The stacktrace produced by the remote server. + */ public Optional getRemoteStackTrace() { return Optional.ofNullable(remoteStackTrace); } + /** + * The list of exceptions generated by the server starting with the top-most one followed by its "cause". That + * cause is then followed by its cause and so on down the line. + */ public Optional> getRemoteExceptionHierarchy() { return Optional.ofNullable(remoteExceptionHierarchy); }