From 7a79d06c921bce8ba51efe61dd4a79b86b8a4198 Mon Sep 17 00:00:00 2001 From: lutovich Date: Mon, 8 Oct 2018 18:40:47 +0200 Subject: [PATCH] Improve stacktraces for errors from blocking API Blocking API works on top of async API and simply blocks on the returned futures. Exceptions can thus contain stacktraces unrelated to the user's code because they originate from driver worker threads. Such exceptions make it hard for users of the driver to determine what has caused the error. This commit makes exceptions thrown from blocking API calls contain stacktraces of the current thread. Original async stacktrace is attached in a dummy suppressed exception. --- .../neo4j/driver/internal/util/ErrorUtil.java | 40 ++++++++++++++++++ .../neo4j/driver/internal/util/Futures.java | 4 +- .../neo4j/driver/v1/integration/ErrorIT.java | 41 +++++++++++++++++++ .../driver/v1/integration/TransactionIT.java | 7 ++-- 4 files changed, 86 insertions(+), 6 deletions(-) diff --git a/driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java b/driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java index f1c40324c3..8e2ae55a5a 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java +++ b/driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java @@ -18,6 +18,11 @@ */ package org.neo4j.driver.internal.util; +import io.netty.util.internal.PlatformDependent; + +import java.util.concurrent.ExecutionException; +import java.util.stream.Stream; + import org.neo4j.driver.v1.exceptions.AuthenticationException; import org.neo4j.driver.v1.exceptions.ClientException; import org.neo4j.driver.v1.exceptions.DatabaseException; @@ -85,6 +90,21 @@ public static boolean isFatal( Throwable error ) return true; } + public static void rethrowAsyncException( ExecutionException e ) + { + Throwable error = e.getCause(); + + InternalExceptionCause internalCause = new InternalExceptionCause( error.getStackTrace() ); + error.addSuppressed( internalCause ); + + StackTraceElement[] currentStackTrace = Stream.of( Thread.currentThread().getStackTrace() ) + .skip( 2 ) // do not include Thread.currentThread() and this method in the stacktrace + .toArray( StackTraceElement[]::new ); + error.setStackTrace( currentStackTrace ); + + PlatformDependent.throwException( error ); + } + private static boolean isProtocolViolationError( Neo4jException error ) { String errorCode = error.code(); @@ -106,4 +126,24 @@ private static String extractClassification( String code ) } return parts[1]; } + + /** + * Exception which is merely a holder of an async stacktrace, which is not the primary stacktrace users are interested in. + * Used for blocking API calls that block on async API calls. + */ + private static class InternalExceptionCause extends RuntimeException + { + InternalExceptionCause( StackTraceElement[] stackTrace ) + { + setStackTrace( stackTrace ); + } + + @Override + public synchronized Throwable fillInStackTrace() + { + // no need to fill in the stack trace + // this exception just uses the given stack trace + return this; + } + } } diff --git a/driver/src/main/java/org/neo4j/driver/internal/util/Futures.java b/driver/src/main/java/org/neo4j/driver/internal/util/Futures.java index a806ae59d7..649be74bd8 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/util/Futures.java +++ b/driver/src/main/java/org/neo4j/driver/internal/util/Futures.java @@ -18,8 +18,6 @@ */ package org.neo4j.driver.internal.util; -import io.netty.util.internal.PlatformDependent; - import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.CompletionStage; @@ -121,7 +119,7 @@ public static V blockingGet( CompletionStage stage, Runnable interruptHan } catch ( ExecutionException e ) { - PlatformDependent.throwException( e.getCause() ); + ErrorUtil.rethrowAsyncException( e ); } } } diff --git a/driver/src/test/java/org/neo4j/driver/v1/integration/ErrorIT.java b/driver/src/test/java/org/neo4j/driver/v1/integration/ErrorIT.java index 18427b9b68..30aef2a2ed 100644 --- a/driver/src/test/java/org/neo4j/driver/v1/integration/ErrorIT.java +++ b/driver/src/test/java/org/neo4j/driver/v1/integration/ErrorIT.java @@ -20,13 +20,18 @@ import io.netty.channel.Channel; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; import org.junit.jupiter.api.extension.RegisterExtension; import java.io.IOException; +import java.lang.reflect.Method; import java.net.URI; +import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.UUID; import java.util.function.Consumer; +import java.util.stream.Stream; import org.neo4j.driver.internal.cluster.RoutingSettings; import org.neo4j.driver.internal.messaging.response.FailureMessage; @@ -46,10 +51,13 @@ import org.neo4j.driver.v1.exceptions.ServiceUnavailableException; import org.neo4j.driver.v1.util.SessionExtension; +import static java.util.Arrays.asList; import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.startsWith; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.junit.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -228,6 +236,20 @@ void shouldCloseChannelOnInboundFatalFailureMessage() throws InterruptedExceptio assertEquals( queryError.getMessage(), errorMessage ); } + @Test + void shouldThrowErrorWithNiceStackTrace( TestInfo testInfo ) + { + ClientException error = assertThrows( ClientException.class, () -> session.run( "RETURN 10 / 0" ).consume() ); + + // thrown error should have this class & method in the stacktrace + StackTraceElement[] stackTrace = error.getStackTrace(); + assertTrue( Stream.of( stackTrace ).anyMatch( element -> testClassAndMethodMatch( testInfo, element ) ), + () -> "Expected stacktrace element is absent:\n" + Arrays.toString( stackTrace ) ); + + // thrown error should have a suppressed error with an async stacktrace + assertThat( asList( error.getSuppressed() ), hasSize( greaterThanOrEqualTo( 1 ) ) ); + } + private Throwable testChannelErrorHandling( Consumer messageFormatSetup ) throws InterruptedException { @@ -276,4 +298,23 @@ private void assertNewQueryCanBeExecuted( Session session, ChannelTrackingDriver Channel lastChannel = channels.get( channels.size() - 1 ); assertTrue( lastChannel.isActive() ); } + + private static boolean testClassAndMethodMatch( TestInfo testInfo, StackTraceElement element ) + { + return testClassMatches( testInfo, element ) && testMethodMatches( testInfo, element ); + } + + private static boolean testClassMatches( TestInfo testInfo, StackTraceElement element ) + { + String expectedName = testInfo.getTestClass().map( Class::getName ).orElse( "" ); + String actualName = element.getClassName(); + return Objects.equals( expectedName, actualName ); + } + + private static boolean testMethodMatches( TestInfo testInfo, StackTraceElement element ) + { + String expectedName = testInfo.getTestMethod().map( Method::getName ).orElse( "" ); + String actualName = element.getMethodName(); + return Objects.equals( expectedName, actualName ); + } } diff --git a/driver/src/test/java/org/neo4j/driver/v1/integration/TransactionIT.java b/driver/src/test/java/org/neo4j/driver/v1/integration/TransactionIT.java index 5153bd8b8a..3945eec68b 100644 --- a/driver/src/test/java/org/neo4j/driver/v1/integration/TransactionIT.java +++ b/driver/src/test/java/org/neo4j/driver/v1/integration/TransactionIT.java @@ -22,7 +22,6 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; -import java.util.Arrays; import java.util.List; import java.util.Map; @@ -45,9 +44,11 @@ import org.neo4j.driver.v1.util.StubServer; import org.neo4j.driver.v1.util.TestUtil; +import static java.util.Arrays.asList; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.startsWith; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.junit.MatcherAssert.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -368,7 +369,7 @@ void shouldDisallowQueriesAfterFailureWhenResultsAreConsumed() try ( Transaction tx = session.beginTransaction() ) { List xs = tx.run( "UNWIND [1,2,3] AS x CREATE (:Node) RETURN x" ).list( record -> record.get( 0 ).asInt() ); - assertEquals( Arrays.asList( 1, 2, 3 ), xs ); + assertEquals( asList( 1, 2, 3 ), xs ); ClientException error1 = assertThrows( ClientException.class, () -> tx.run( "RETURN unknown" ).consume() ); assertThat( error1.code(), containsString( "SyntaxError" ) ); @@ -404,7 +405,7 @@ void shouldRollbackWhenMarkedSuccessfulButOneStatementFails() } ); assertThat( error.code(), containsString( "SyntaxError" ) ); - assertEquals( 1, error.getSuppressed().length ); + assertThat( error.getSuppressed().length, greaterThanOrEqualTo( 1 ) ); Throwable suppressed = error.getSuppressed()[0]; assertThat( suppressed, instanceOf( ClientException.class ) ); assertThat( suppressed.getMessage(), startsWith( "Transaction can't be committed" ) );