Skip to content

Commit fb50aa0

Browse files
committed
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.
1 parent d6f1f86 commit fb50aa0

File tree

3 files changed

+82
-3
lines changed

3 files changed

+82
-3
lines changed

driver/src/main/java/org/neo4j/driver/internal/util/ErrorUtil.java

+40
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@
1818
*/
1919
package org.neo4j.driver.internal.util;
2020

21+
import io.netty.util.internal.PlatformDependent;
22+
23+
import java.util.concurrent.ExecutionException;
24+
import java.util.stream.Stream;
25+
2126
import org.neo4j.driver.v1.exceptions.AuthenticationException;
2227
import org.neo4j.driver.v1.exceptions.ClientException;
2328
import org.neo4j.driver.v1.exceptions.DatabaseException;
@@ -85,6 +90,21 @@ public static boolean isFatal( Throwable error )
8590
return true;
8691
}
8792

93+
public static void rethrowAsyncException( ExecutionException e )
94+
{
95+
Throwable error = e.getCause();
96+
97+
InternalExceptionCause internalCause = new InternalExceptionCause( error.getStackTrace() );
98+
error.addSuppressed( internalCause );
99+
100+
StackTraceElement[] currentStackTrace = Stream.of( Thread.currentThread().getStackTrace() )
101+
.skip( 2 ) // do not include Thread.currentThread() and this method in the stacktrace
102+
.toArray( StackTraceElement[]::new );
103+
error.setStackTrace( currentStackTrace );
104+
105+
PlatformDependent.throwException( error );
106+
}
107+
88108
private static boolean isProtocolViolationError( Neo4jException error )
89109
{
90110
String errorCode = error.code();
@@ -106,4 +126,24 @@ private static String extractClassification( String code )
106126
}
107127
return parts[1];
108128
}
129+
130+
/**
131+
* Exception which is merely a holder of an async stacktrace, which is not the primary stacktrace users are interested in.
132+
* Used for blocking API calls that block on async API calls.
133+
*/
134+
private static class InternalExceptionCause extends RuntimeException
135+
{
136+
InternalExceptionCause( StackTraceElement[] stackTrace )
137+
{
138+
setStackTrace( stackTrace );
139+
}
140+
141+
@Override
142+
public synchronized Throwable fillInStackTrace()
143+
{
144+
// no need to fill in the stack trace
145+
// this exception just uses the given stack trace
146+
return this;
147+
}
148+
}
109149
}

driver/src/main/java/org/neo4j/driver/internal/util/Futures.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@
1818
*/
1919
package org.neo4j.driver.internal.util;
2020

21-
import io.netty.util.internal.PlatformDependent;
22-
2321
import java.util.concurrent.CompletableFuture;
2422
import java.util.concurrent.CompletionException;
2523
import java.util.concurrent.CompletionStage;
@@ -121,7 +119,7 @@ public static <V> V blockingGet( CompletionStage<V> stage, Runnable interruptHan
121119
}
122120
catch ( ExecutionException e )
123121
{
124-
PlatformDependent.throwException( e.getCause() );
122+
ErrorUtil.rethrowAsyncException( e );
125123
}
126124
}
127125
}

driver/src/test/java/org/neo4j/driver/v1/integration/ErrorIT.java

+41
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,18 @@
2020

2121
import io.netty.channel.Channel;
2222
import org.junit.jupiter.api.Test;
23+
import org.junit.jupiter.api.TestInfo;
2324
import org.junit.jupiter.api.extension.RegisterExtension;
2425

2526
import java.io.IOException;
27+
import java.lang.reflect.Method;
2628
import java.net.URI;
29+
import java.util.Arrays;
2730
import java.util.List;
31+
import java.util.Objects;
2832
import java.util.UUID;
2933
import java.util.function.Consumer;
34+
import java.util.stream.Stream;
3035

3136
import org.neo4j.driver.internal.cluster.RoutingSettings;
3237
import org.neo4j.driver.internal.messaging.response.FailureMessage;
@@ -46,10 +51,13 @@
4651
import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;
4752
import org.neo4j.driver.v1.util.SessionExtension;
4853

54+
import static java.util.Arrays.asList;
4955
import static java.util.concurrent.TimeUnit.SECONDS;
5056
import static org.hamcrest.CoreMatchers.equalTo;
5157
import static org.hamcrest.CoreMatchers.startsWith;
5258
import static org.hamcrest.Matchers.containsString;
59+
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
60+
import static org.hamcrest.Matchers.hasSize;
5361
import static org.hamcrest.Matchers.instanceOf;
5462
import static org.hamcrest.junit.MatcherAssert.assertThat;
5563
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -228,6 +236,20 @@ void shouldCloseChannelOnInboundFatalFailureMessage() throws InterruptedExceptio
228236
assertEquals( queryError.getMessage(), errorMessage );
229237
}
230238

239+
@Test
240+
void shouldThrowErrorWithNiceStackTrace( TestInfo testInfo )
241+
{
242+
ClientException error = assertThrows( ClientException.class, () -> session.run( "RETURN 10 / 0" ).consume() );
243+
244+
// thrown error should have this class & method in the stacktrace
245+
StackTraceElement[] stackTrace = error.getStackTrace();
246+
assertTrue( Stream.of( stackTrace ).anyMatch( element -> testClassAndMethodMatch( testInfo, element ) ),
247+
() -> "Expected stacktrace element is absent:\n" + Arrays.toString( stackTrace ) );
248+
249+
// thrown error should have a suppressed error with an async stacktrace
250+
assertThat( asList( error.getSuppressed() ), hasSize( greaterThanOrEqualTo( 1 ) ) );
251+
}
252+
231253
private Throwable testChannelErrorHandling( Consumer<FailingMessageFormat> messageFormatSetup )
232254
throws InterruptedException
233255
{
@@ -276,4 +298,23 @@ private void assertNewQueryCanBeExecuted( Session session, ChannelTrackingDriver
276298
Channel lastChannel = channels.get( channels.size() - 1 );
277299
assertTrue( lastChannel.isActive() );
278300
}
301+
302+
private static boolean testClassAndMethodMatch( TestInfo testInfo, StackTraceElement element )
303+
{
304+
return testClassMatches( testInfo, element ) && testMethodMatches( testInfo, element );
305+
}
306+
307+
private static boolean testClassMatches( TestInfo testInfo, StackTraceElement element )
308+
{
309+
String expectedName = testInfo.getTestClass().map( Class::getName ).orElse( "" );
310+
String actualName = element.getClassName();
311+
return Objects.equals( expectedName, actualName );
312+
}
313+
314+
private static boolean testMethodMatches( TestInfo testInfo, StackTraceElement element )
315+
{
316+
String expectedName = testInfo.getTestMethod().map( Method::getName ).orElse( "" );
317+
String actualName = element.getMethodName();
318+
return Objects.equals( expectedName, actualName );
319+
}
279320
}

0 commit comments

Comments
 (0)