Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve stacktraces for errors from blocking API #538

Merged
merged 1 commit into from
Oct 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -121,7 +119,7 @@ public static <V> V blockingGet( CompletionStage<V> stage, Runnable interruptHan
}
catch ( ExecutionException e )
{
PlatformDependent.throwException( e.getCause() );
ErrorUtil.rethrowAsyncException( e );
}
}
}
Expand Down
41 changes: 41 additions & 0 deletions driver/src/test/java/org/neo4j/driver/v1/integration/ErrorIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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 );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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" ) );
Expand Down Expand Up @@ -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" ) );
Expand Down