Skip to content

Commit 7cb0359

Browse files
bigmontzbigmontz
committed
Remove circular references in exceptions during the commit
The error issue happened because the cursor failure is already the cause of the commit or rollback failure. This generates a circular reference when both of the exceptions are combined. This situation can create stack overflow when the exception is logged in some logging libraries. The solution don't add the cursor exception as the cause of the exception, this way it will not be wrongly combined creating a circular reference.
1 parent a179e47 commit 7cb0359

File tree

9 files changed

+228
-22
lines changed

9 files changed

+228
-22
lines changed

‎driver/src/main/java/org/neo4j/driver/internal/async/UnmanagedTransaction.java

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -121,24 +121,29 @@ boolean isOpen()
121121

122122
private volatile StateHolder state = StateHolder.of( State.ACTIVE );
123123

124-
public UnmanagedTransaction(Connection connection, BookmarkHolder bookmarkHolder, long fetchSize )
124+
public UnmanagedTransaction( Connection connection, BookmarkHolder bookmarkHolder, long fetchSize )
125+
{
126+
this( connection, bookmarkHolder, fetchSize, new ResultCursorsHolder() );
127+
}
128+
129+
protected UnmanagedTransaction( Connection connection, BookmarkHolder bookmarkHolder, long fetchSize, ResultCursorsHolder resultCursors )
125130
{
126131
this.connection = connection;
127132
this.protocol = connection.protocol();
128133
this.bookmarkHolder = bookmarkHolder;
129-
this.resultCursors = new ResultCursorsHolder();
134+
this.resultCursors = resultCursors;
130135
this.fetchSize = fetchSize;
131136
}
132137

133-
public CompletionStage<UnmanagedTransaction> beginAsync(Bookmark initialBookmark, TransactionConfig config )
138+
public CompletionStage<UnmanagedTransaction> beginAsync( Bookmark initialBookmark, TransactionConfig config )
134139
{
135140
return protocol.beginTransaction( connection, initialBookmark, config )
136-
.handle( ( ignore, beginError ) ->
137-
{
138-
if ( beginError != null )
139-
{
140-
if ( beginError instanceof AuthorizationExpiredException )
141-
{
141+
.handle( ( ignore, beginError ) ->
142+
{
143+
if ( beginError != null )
144+
{
145+
if ( beginError instanceof AuthorizationExpiredException )
146+
{
142147
connection.terminateAndRelease( AuthorizationExpiredException.DESCRIPTION );
143148
}
144149
else
@@ -176,7 +181,7 @@ else if ( state.value == State.ROLLED_BACK )
176181
else
177182
{
178183
return resultCursors.retrieveNotConsumedError()
179-
.thenCompose( error -> doCommitAsync().handle( handleCommitOrRollback( error ) ) )
184+
.thenCompose( error -> doCommitAsync( error ).handle( handleCommitOrRollback( error ) ) )
180185
.whenComplete( ( ignore, error ) -> handleTransactionCompletion( State.COMMITTED, error ) );
181186
}
182187
}
@@ -249,12 +254,13 @@ else if ( state.value == State.TERMINATED )
249254
}
250255
}
251256

252-
private CompletionStage<Void> doCommitAsync()
257+
private CompletionStage<Void> doCommitAsync( Throwable cursorFailure )
253258
{
254259
if ( state.value == State.TERMINATED )
255260
{
256261
return failedFuture( new ClientException( "Transaction can't be committed. " +
257-
"It has been rolled back either because of an error or explicit termination", state.causeOfTermination ) );
262+
"It has been rolled back either because of an error or explicit termination",
263+
cursorFailure != state.causeOfTermination ? state.causeOfTermination : null ) );
258264
}
259265
return protocol.commitTransaction( connection ).thenAccept( bookmarkHolder::setBookmark );
260266
}

‎driver/src/test/java/org/neo4j/driver/integration/BookmarkIT.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,17 @@
1818
*/
1919
package org.neo4j.driver.integration;
2020

21-
import org.hamcrest.BaseMatcher;
22-
import org.hamcrest.Description;
2321
import org.junit.jupiter.api.BeforeEach;
2422
import org.junit.jupiter.api.Test;
2523
import org.junit.jupiter.api.extension.RegisterExtension;
2624

2725
import java.util.UUID;
2826

27+
import org.neo4j.driver.Bookmark;
2928
import org.neo4j.driver.Driver;
3029
import org.neo4j.driver.Session;
3130
import org.neo4j.driver.Transaction;
3231
import org.neo4j.driver.exceptions.ClientException;
33-
import org.neo4j.driver.Bookmark;
34-
import org.neo4j.driver.internal.InternalBookmark;
3532
import org.neo4j.driver.internal.util.DisabledOnNeo4jWith;
3633
import org.neo4j.driver.internal.util.EnabledOnNeo4jWith;
3734
import org.neo4j.driver.internal.util.Neo4jFeature;
@@ -47,6 +44,7 @@
4744
import static org.neo4j.driver.internal.util.BookmarkUtil.assertBookmarkContainsSingleValue;
4845
import static org.neo4j.driver.internal.util.BookmarkUtil.assertBookmarkIsEmpty;
4946
import static org.neo4j.driver.internal.util.BookmarkUtil.assertBookmarksContainsSingleUniqueValues;
47+
import static org.neo4j.driver.util.TestUtil.assertNoCircularReferences;
5048

5149
@ParallelizableIT
5250
class BookmarkIT
@@ -137,7 +135,8 @@ void bookmarkRemainsAfterTxFailure()
137135
Transaction tx = session.beginTransaction();
138136
tx.run( "RETURN" );
139137

140-
assertThrows( ClientException.class, tx::commit );
138+
ClientException e = assertThrows( ClientException.class, tx::commit );
139+
assertNoCircularReferences( e );
141140
assertEquals( bookmark, session.lastBookmark() );
142141
}
143142

‎driver/src/test/java/org/neo4j/driver/integration/QueryIT.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,13 @@
1818
*/
1919
package org.neo4j.driver.integration;
2020

21+
import org.junit.jupiter.api.Assertions;
2122
import org.junit.jupiter.api.Test;
2223
import org.junit.jupiter.api.extension.RegisterExtension;
24+
import org.junit.jupiter.api.function.Executable;
2325

26+
import java.io.PrintWriter;
27+
import java.io.StringWriter;
2428
import java.util.Collections;
2529
import java.util.Iterator;
2630
import java.util.List;
@@ -36,8 +40,10 @@
3640
import static java.util.Arrays.asList;
3741
import static org.hamcrest.CoreMatchers.equalTo;
3842
import static org.hamcrest.MatcherAssert.assertThat;
43+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
3944
import static org.junit.jupiter.api.Assertions.assertThrows;
4045
import static org.neo4j.driver.Values.parameters;
46+
import static org.neo4j.driver.util.TestUtil.assertNoCircularReferences;
4147

4248
@ParallelizableIT
4349
class QueryIT
@@ -182,4 +188,18 @@ void shouldFailForIllegalQueries()
182188
assertThrows( IllegalArgumentException.class, () -> session.run( (String) null ) );
183189
assertThrows( IllegalArgumentException.class, () -> session.run( "" ) );
184190
}
191+
192+
@Test
193+
void shouldBeAbleToLogSemanticWrongExceptions() {
194+
try {
195+
// When I run a query with the old syntax
196+
session.writeTransaction(tx ->
197+
tx.run( "MATCH (n:Element) WHERE n.name = {param} RETURN n",
198+
parameters("param", "Luke" )).list());
199+
} catch ( Exception ex ) {
200+
// And exception happens
201+
// Then it should not have circular reference
202+
assertNoCircularReferences(ex);
203+
}
204+
}
185205
}

‎driver/src/test/java/org/neo4j/driver/integration/TransactionIT.java

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@
2929
import org.neo4j.driver.Config;
3030
import org.neo4j.driver.Driver;
3131
import org.neo4j.driver.Record;
32-
import org.neo4j.driver.Session;
3332
import org.neo4j.driver.Result;
33+
import org.neo4j.driver.Session;
3434
import org.neo4j.driver.Transaction;
3535
import org.neo4j.driver.Value;
3636
import org.neo4j.driver.exceptions.ClientException;
@@ -57,6 +57,7 @@
5757
import static org.junit.jupiter.api.Assertions.assertTrue;
5858
import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING;
5959
import static org.neo4j.driver.internal.retry.RetrySettings.DEFAULT;
60+
import static org.neo4j.driver.util.TestUtil.assertNoCircularReferences;
6061

6162
@ParallelizableIT
6263
class TransactionIT
@@ -291,7 +292,8 @@ void shouldRollBackTxIfErrorWithoutConsume()
291292
Transaction tx = session.beginTransaction();
292293
tx.run( "invalid" ); // send run, pull_all
293294

294-
assertThrows( ClientException.class, tx::commit );
295+
ClientException e = assertThrows( ClientException.class, tx::commit );
296+
assertNoCircularReferences( e );
295297

296298
try ( Transaction anotherTx = session.beginTransaction() )
297299
{
@@ -385,7 +387,8 @@ void shouldBeResponsiveToThreadInterruptWhenWaitingForCommit()
385387

386388
try
387389
{
388-
assertThrows( ServiceUnavailableException.class, tx2::commit );
390+
ServiceUnavailableException e = assertThrows( ServiceUnavailableException.class, tx2::commit );
391+
assertNoCircularReferences( e );
389392
}
390393
finally
391394
{
@@ -481,6 +484,7 @@ void shouldRollbackWhenMarkedSuccessfulButOneQueryFails()
481484
}
482485
} );
483486

487+
assertNoCircularReferences( error );
484488
assertThat( error.code(), containsString( "SyntaxError" ) );
485489
assertThat( error.getSuppressed().length, greaterThanOrEqualTo( 1 ) );
486490
Throwable suppressed = error.getSuppressed()[0];

‎driver/src/test/java/org/neo4j/driver/integration/async/AsyncQueryIT.java

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
/*
2+
* Copyright (c) "Neo4j"
3+
* Neo4j Sweden AB [http://neo4j.com]
4+
*
5+
* This file is part of Neo4j.
6+
*
7+
* Licensed under the Apache License, Version 2.0 (the "License");
8+
* you may not use this file except in compliance with the License.
9+
* You may obtain a copy of the License at
10+
*
11+
* http://www.apache.org/licenses/LICENSE-2.0
12+
*
13+
* Unless required by applicable law or agreed to in writing, software
14+
* distributed under the License is distributed on an "AS IS" BASIS,
15+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
* See the License for the specific language governing permissions and
17+
* limitations under the License.
18+
*/
19+
package org.neo4j.driver.integration.async;
20+
21+
import org.junit.jupiter.api.AfterEach;
22+
import org.junit.jupiter.api.Assertions;
23+
import org.junit.jupiter.api.BeforeEach;
24+
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.api.extension.RegisterExtension;
26+
import org.slf4j.Logger;
27+
import org.slf4j.LoggerFactory;
28+
import reactor.core.publisher.Flux;
29+
import reactor.core.publisher.Mono;
30+
31+
import java.io.PrintWriter;
32+
import java.io.StringWriter;
33+
import java.util.ArrayList;
34+
import java.util.concurrent.ExecutionException;
35+
36+
import org.neo4j.driver.async.AsyncSession;
37+
import org.neo4j.driver.util.DatabaseExtension;
38+
import org.neo4j.driver.util.ParallelizableIT;
39+
40+
import static org.neo4j.driver.Values.parameters;
41+
import static org.neo4j.driver.util.TestUtil.assertNoCircularReferences;
42+
43+
@ParallelizableIT
44+
public class AsyncQueryIT
45+
{
46+
private static final Logger LOGGER = LoggerFactory.getLogger( AsyncQueryIT.class );
47+
48+
@RegisterExtension
49+
static final DatabaseExtension neo4j = new DatabaseExtension();
50+
51+
private AsyncSession session;
52+
53+
@BeforeEach
54+
void setUp()
55+
{
56+
session = neo4j.driver().asyncSession();
57+
}
58+
59+
@AfterEach
60+
void tearDown()
61+
{
62+
session.closeAsync();
63+
}
64+
65+
@Test
66+
void shouldBeAbleToLogSemanticWrongExceptions() throws ExecutionException, InterruptedException
67+
{
68+
session.writeTransactionAsync( tx -> Flux.from(
69+
Mono.fromCompletionStage(
70+
tx.runAsync( "MATCH (n:Element) WHERE n.name = {param} RETURN n", parameters("param", "Luke") )
71+
)).collectList().toFuture())
72+
73+
.toCompletableFuture()
74+
.exceptionally( ex -> {
75+
assertNoCircularReferences(ex);
76+
return new ArrayList<>();
77+
} )
78+
.get();
79+
}
80+
81+
}

‎driver/src/test/java/org/neo4j/driver/integration/async/AsyncSessionIT.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
import static org.neo4j.driver.internal.util.Matchers.syntaxError;
8484
import static org.neo4j.driver.internal.util.Neo4jFeature.BOLT_V3;
8585
import static org.neo4j.driver.internal.util.Neo4jFeature.BOLT_V4;
86+
import static org.neo4j.driver.util.TestUtil.assertNoCircularReferences;
8687
import static org.neo4j.driver.util.TestUtil.await;
8788
import static org.neo4j.driver.util.TestUtil.awaitAll;
8889

@@ -381,7 +382,8 @@ void shouldRunAsyncTransactionThatCanNotBeRetried()
381382
InvocationTrackingWork work = new InvocationTrackingWork( "UNWIND [10, 5, 0] AS x CREATE (:Hi) RETURN 10/x" );
382383
CompletionStage<Record> txStage = session.writeTransactionAsync( work );
383384

384-
assertThrows( ClientException.class, () -> await( txStage ) );
385+
ClientException e = assertThrows( ClientException.class, () -> await( txStage ) );
386+
assertNoCircularReferences( e );
385387
assertEquals( 1, work.invocationCount() );
386388
assertEquals( 0, countNodesByLabel( "Hi" ) );
387389
}

‎driver/src/test/java/org/neo4j/driver/integration/async/AsyncTransactionIT.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
import static org.neo4j.driver.internal.util.Iterables.single;
7070
import static org.neo4j.driver.internal.util.Matchers.containsResultAvailableAfterAndResultConsumedAfter;
7171
import static org.neo4j.driver.internal.util.Matchers.syntaxError;
72+
import static org.neo4j.driver.util.TestUtil.assertNoCircularReferences;
7273
import static org.neo4j.driver.util.TestUtil.await;
7374

7475
@ParallelizableIT
@@ -677,6 +678,7 @@ void shouldFailToCommitWhenQueriesFailAndErrorNotConsumed() throws InterruptedEx
677678
tx.runAsync( "CREATE (:TestNode)" );
678679

679680
ClientException e = assertThrows( ClientException.class, () -> await( tx.commitAsync() ) );
681+
assertNoCircularReferences( e );
680682
assertEquals( "/ by zero", e.getMessage() );
681683
}
682684

@@ -688,6 +690,7 @@ void shouldPropagateRunFailureFromCommit()
688690
tx.runAsync( "RETURN ILLEGAL" );
689691

690692
ClientException e = assertThrows( ClientException.class, () -> await( tx.commitAsync() ) );
693+
assertNoCircularReferences( e );
691694
assertThat( e.getMessage(), containsString( "ILLEGAL" ) );
692695
}
693696

@@ -699,6 +702,7 @@ void shouldPropagateBlockedRunFailureFromCommit()
699702
await( tx.runAsync( "RETURN 42 / 0" ) );
700703

701704
ClientException e = assertThrows( ClientException.class, () -> await( tx.commitAsync() ) );
705+
assertNoCircularReferences( e );
702706
assertThat( e.getMessage(), containsString( "/ by zero" ) );
703707
}
704708

@@ -732,6 +736,7 @@ void shouldPropagatePullAllFailureFromCommit()
732736
tx.runAsync( "UNWIND [1, 2, 3, 'Hi'] AS x RETURN 10 / x" );
733737

734738
ClientException e = assertThrows( ClientException.class, () -> await( tx.commitAsync() ) );
739+
assertNoCircularReferences( e );
735740
assertThat( e.code(), containsString( "TypeError" ) );
736741
}
737742

@@ -743,6 +748,7 @@ void shouldPropagateBlockedPullAllFailureFromCommit()
743748
await( tx.runAsync( "UNWIND [1, 2, 3, 'Hi'] AS x RETURN 10 / x" ) );
744749

745750
ClientException e = assertThrows( ClientException.class, () -> await( tx.commitAsync() ) );
751+
assertNoCircularReferences( e );
746752
assertThat( e.code(), containsString( "TypeError" ) );
747753
}
748754

0 commit comments

Comments
 (0)