Merge pull request #538 from lutovich/1.7-async-stacktraces

Improve stacktraces for errors from blocking API

Author: ali-ince

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;
+        }
+    }
 }

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> V blockingGet( CompletionStage<V> stage, Runnable interruptHan
                 }
                 catch ( ExecutionException e )
                 {
-                    PlatformDependent.throwException( e.getCause() );
+                    ErrorUtil.rethrowAsyncException( e );
                 }
             }
         }

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<FailingMessageFormat> 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 );
+    }
 }

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<Integer> 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" ) );