From 6b9256ff24875d296171dd961b261a9c8df8acc9 Mon Sep 17 00:00:00 2001 From: Dmitriy Tverdiakov Date: Fri, 13 Aug 2021 14:20:19 +0100 Subject: [PATCH] Migrate tests from Java Driver to Testkit New features: - `Temporary:DriverMaxTxRetryTime` - configures maximum transaction retry time. Replaced tests: - shouldCloseChannelWhenResetFails -> ChannelErrorHandler.channelInactive (existing unit test) Migrated tests: - shouldRetryReadTransactionUntilFailure -> test_should_fail_when_reading_from_unexpectedly_interrupting_readers_using_tx_function - shouldRetryWriteTransactionUntilFailure -> test_should_fail_when_writing_to_unexpectedly_interrupting_writers_using_tx_function - useSessionAfterDriverIsClosed -> test_should_fail_when_driver_closed_using_session_run --- .../async/pool/ConnectionPoolImpl.java | 4 +- .../integration/RoutingDriverBoltKitIT.java | 119 ------------------ .../internal/DirectDriverBoltKitIT.java | 44 ------- .../internal/TrustedServerProductTest.java | 50 -------- .../resources/acquire_endpoints_v3.script | 10 -- .../test/resources/dead_read_server_tx.script | 9 -- .../test/resources/dead_write_server.script | 9 -- .../test/resources/read_server_v3_read.script | 12 -- driver/src/test/resources/reset_error.script | 12 -- .../test/resources/untrusted_server.script | 6 - .../org/testkit/backend/CommandProcessor.java | 22 ++++ .../messages/requests/GetFeatures.java | 3 +- .../backend/messages/requests/NewDriver.java | 10 +- 13 files changed, 34 insertions(+), 276 deletions(-) delete mode 100644 driver/src/test/java/org/neo4j/driver/internal/TrustedServerProductTest.java delete mode 100644 driver/src/test/resources/acquire_endpoints_v3.script delete mode 100644 driver/src/test/resources/dead_read_server_tx.script delete mode 100644 driver/src/test/resources/dead_write_server.script delete mode 100644 driver/src/test/resources/read_server_v3_read.script delete mode 100644 driver/src/test/resources/reset_error.script delete mode 100644 driver/src/test/resources/untrusted_server.script diff --git a/driver/src/main/java/org/neo4j/driver/internal/async/pool/ConnectionPoolImpl.java b/driver/src/main/java/org/neo4j/driver/internal/async/pool/ConnectionPoolImpl.java index 03c7b8a2d2..b1ef41bbb3 100644 --- a/driver/src/main/java/org/neo4j/driver/internal/async/pool/ConnectionPoolImpl.java +++ b/driver/src/main/java/org/neo4j/driver/internal/async/pool/ConnectionPoolImpl.java @@ -52,6 +52,8 @@ public class ConnectionPoolImpl implements ConnectionPool { + public static final String CONNECTION_POOL_CLOSED_ERROR_MESSAGE = "Pool closed"; + private final ChannelConnector connector; private final Bootstrap bootstrap; private final NettyChannelTracker nettyChannelTracker; @@ -227,7 +229,7 @@ private void assertNotClosed() { if ( closed.get() ) { - throw new IllegalStateException( "Pool closed" ); + throw new IllegalStateException( CONNECTION_POOL_CLOSED_ERROR_MESSAGE ); } } diff --git a/driver/src/test/java/org/neo4j/driver/integration/RoutingDriverBoltKitIT.java b/driver/src/test/java/org/neo4j/driver/integration/RoutingDriverBoltKitIT.java index 055056c1cc..196ec8bee6 100644 --- a/driver/src/test/java/org/neo4j/driver/integration/RoutingDriverBoltKitIT.java +++ b/driver/src/test/java/org/neo4j/driver/integration/RoutingDriverBoltKitIT.java @@ -27,27 +27,14 @@ import java.io.IOException; import java.net.URI; -import java.util.Comparator; import java.util.List; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; -import org.neo4j.driver.AccessMode; -import org.neo4j.driver.AuthToken; -import org.neo4j.driver.AuthTokens; import org.neo4j.driver.Config; import org.neo4j.driver.Driver; import org.neo4j.driver.GraphDatabase; import org.neo4j.driver.Record; -import org.neo4j.driver.Session; -import org.neo4j.driver.TransactionWork; import org.neo4j.driver.async.AsyncSession; -import org.neo4j.driver.exceptions.SessionExpiredException; -import org.neo4j.driver.internal.DriverFactory; -import org.neo4j.driver.internal.cluster.RoutingSettings; -import org.neo4j.driver.internal.retry.RetrySettings; -import org.neo4j.driver.internal.security.SecurityPlanImpl; -import org.neo4j.driver.internal.util.DriverFactoryWithFixedRetryLogic; import org.neo4j.driver.internal.util.Futures; import org.neo4j.driver.net.ServerAddress; import org.neo4j.driver.net.ServerAddressResolver; @@ -66,7 +53,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.neo4j.driver.SessionConfig.builder; -import static org.neo4j.driver.util.StubServer.INSECURE_CONFIG; import static org.neo4j.driver.util.StubServer.insecureBuilder; /** @@ -166,50 +152,6 @@ void shouldHandleLeaderSwitchAndRetryWhenWritingInTxFunctionRX() throws IOExcept assertThat( writeServer.exitStatus(), equalTo( 0 ) ); } - // fixed retries are not currently supported in testkit - @Test - void shouldRetryReadTransactionUntilFailure() throws Exception - { - StubServer router = stubController.startStub( "acquire_endpoints_v3.script", 9001 ); - StubServer brokenReader1 = stubController.startStub( "dead_read_server_tx.script", 9005 ); - StubServer brokenReader2 = stubController.startStub( "dead_read_server_tx.script", 9006 ); - - try ( Driver driver = newDriverWithFixedRetries( "neo4j://127.0.0.1:9001", 1 ); Session session = driver.session() ) - { - AtomicInteger invocations = new AtomicInteger(); - assertThrows( SessionExpiredException.class, () -> session.readTransaction( queryWork( "MATCH (n) RETURN n.name", invocations ) ) ); - assertEquals( 2, invocations.get() ); - } - finally - { - assertEquals( 0, router.exitStatus() ); - assertEquals( 0, brokenReader1.exitStatus() ); - assertEquals( 0, brokenReader2.exitStatus() ); - } - } - - // fixed retries are not currently supported in testkit - @Test - void shouldRetryWriteTransactionUntilFailure() throws Exception - { - StubServer router = stubController.startStub( "acquire_endpoints_v3.script", 9001 ); - StubServer brokenWriter1 = stubController.startStub( "dead_write_server.script", 9007 ); - StubServer brokenWriter2 = stubController.startStub( "dead_write_server.script", 9008 ); - - try ( Driver driver = newDriverWithFixedRetries( "neo4j://127.0.0.1:9001", 1 ); Session session = driver.session() ) - { - AtomicInteger invocations = new AtomicInteger(); - assertThrows( SessionExpiredException.class, () -> session.writeTransaction( queryWork( "CREATE (n {name:'Bob'})", invocations ) ) ); - assertEquals( 2, invocations.get() ); - } - finally - { - assertEquals( 0, router.exitStatus() ); - assertEquals( 0, brokenWriter1.exitStatus() ); - assertEquals( 0, brokenWriter2.exitStatus() ); - } - } - @Test void shouldFailInitialDiscoveryWhenConfiguredResolverThrows() { @@ -223,65 +165,4 @@ void shouldFailInitialDiscoveryWhenConfiguredResolverThrows() assertEquals( "Resolution failure!", error.getMessage() ); verify( resolver ).resolve( ServerAddress.of( "my.server.com", 9001 ) ); } - - // general error reporting and handling should be improved before this can be moved to testkit - // also, backend closes socket on general errors and it negatively impacts testkit's teardown process - @Test - void useSessionAfterDriverIsClosed() throws Exception - { - StubServer router = stubController.startStub( "acquire_endpoints_v3.script", 9001 ); - StubServer readServer = stubController.startStub( "read_server_v3_read.script", 9005 ); - - try ( Driver driver = GraphDatabase.driver( "neo4j://127.0.0.1:9001", INSECURE_CONFIG ) ) - { - try ( Session session = driver.session( builder().withDefaultAccessMode( AccessMode.READ ).build() ) ) - { - List records = session.run( "MATCH (n) RETURN n.name" ).list(); - assertEquals( 3, records.size() ); - } - - Session session = driver.session( builder().withDefaultAccessMode( AccessMode.READ ).build() ); - - driver.close(); - - assertThrows( IllegalStateException.class, () -> session.run( "MATCH (n) RETURN n.name" ) ); - } - finally - { - assertEquals( 0, readServer.exitStatus() ); - assertEquals( 0, router.exitStatus() ); - } - } - - private static Driver newDriverWithFixedRetries( String uriString, int retries ) - { - DriverFactory driverFactory = new DriverFactoryWithFixedRetryLogic( retries ); - return newDriver( uriString, driverFactory, INSECURE_CONFIG ); - } - - private static Driver newDriver( String uriString, DriverFactory driverFactory, Config config ) - { - URI uri = URI.create( uriString ); - RoutingSettings routingConf = new RoutingSettings( 1, 1, 0, null ); - AuthToken auth = AuthTokens.none(); - return driverFactory.newInstance( uri, auth, routingConf, RetrySettings.DEFAULT, config, SecurityPlanImpl.insecure() ); - } - - private static TransactionWork> queryWork( final String query, final AtomicInteger invocations ) - { - return tx -> - { - invocations.incrementAndGet(); - return tx.run( query ).list(); - }; - } - - static class PortBasedServerAddressComparator implements Comparator - { - @Override - public int compare( ServerAddress a1, ServerAddress a2 ) - { - return Integer.compare( a1.port(), a2.port() ); - } - } } diff --git a/driver/src/test/java/org/neo4j/driver/internal/DirectDriverBoltKitIT.java b/driver/src/test/java/org/neo4j/driver/internal/DirectDriverBoltKitIT.java index 47edc42b30..3fb15a5d57 100644 --- a/driver/src/test/java/org/neo4j/driver/internal/DirectDriverBoltKitIT.java +++ b/driver/src/test/java/org/neo4j/driver/internal/DirectDriverBoltKitIT.java @@ -18,7 +18,6 @@ */ package org.neo4j.driver.internal; -import io.netty.channel.Channel; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -26,22 +25,13 @@ import reactor.core.publisher.Mono; import reactor.test.StepVerifier; -import java.net.URI; import java.util.ArrayList; import java.util.List; -import org.neo4j.driver.AuthTokens; -import org.neo4j.driver.Config; import org.neo4j.driver.Driver; import org.neo4j.driver.GraphDatabase; -import org.neo4j.driver.Session; import org.neo4j.driver.async.AsyncSession; import org.neo4j.driver.async.ResultCursor; -import org.neo4j.driver.internal.cluster.RoutingSettings; -import org.neo4j.driver.internal.retry.RetrySettings; -import org.neo4j.driver.internal.security.SecurityPlanImpl; -import org.neo4j.driver.internal.util.Clock; -import org.neo4j.driver.internal.util.io.ChannelTrackingDriverFactory; import org.neo4j.driver.reactive.RxResult; import org.neo4j.driver.reactive.RxSession; import org.neo4j.driver.util.StubServer; @@ -49,11 +39,8 @@ import static java.util.Arrays.asList; import static java.util.Collections.singletonList; -import static java.util.concurrent.TimeUnit.SECONDS; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.neo4j.driver.SessionConfig.builder; -import static org.neo4j.driver.internal.logging.DevNullLogging.DEV_NULL_LOGGING; import static org.neo4j.driver.util.StubServer.INSECURE_CONFIG; import static org.neo4j.driver.util.TestUtil.await; @@ -73,37 +60,6 @@ public void killServers() stubController.reset(); } - @Test - void shouldCloseChannelWhenResetFails() throws Exception - { - StubServer server = stubController.startStub( "reset_error.script", 9001 ); - try - { - URI uri = URI.create( "bolt://localhost:9001" ); - Config config = Config.builder().withLogging( DEV_NULL_LOGGING ).withoutEncryption().build(); - ChannelTrackingDriverFactory driverFactory = new ChannelTrackingDriverFactory( 1, Clock.SYSTEM ); - - try ( Driver driver = driverFactory.newInstance( uri, AuthTokens.none(), RoutingSettings.DEFAULT, RetrySettings.DEFAULT, config, - SecurityPlanImpl.insecure() ) ) - { - try ( Session session = driver.session() ) - { - assertEquals( 42, session.run( "RETURN 42 AS answer" ).single().get( 0 ).asInt() ); - } - - List channels = driverFactory.pollChannels(); - // there should be a single channel - assertEquals( 1, channels.size() ); - // and it should be closed because it failed to RESET - assertNull( channels.get( 0 ).closeFuture().get( 30, SECONDS ) ); - } - } - finally - { - assertEquals( 0, server.exitStatus() ); - } - } - @Test void shouldStreamingRecordsInBatchesRx() throws Exception { diff --git a/driver/src/test/java/org/neo4j/driver/internal/TrustedServerProductTest.java b/driver/src/test/java/org/neo4j/driver/internal/TrustedServerProductTest.java deleted file mode 100644 index 7e473c3389..0000000000 --- a/driver/src/test/java/org/neo4j/driver/internal/TrustedServerProductTest.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright (c) "Neo4j" - * Neo4j Sweden AB [http://neo4j.com] - * - * This file is part of Neo4j. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.neo4j.driver.internal; - -import org.junit.jupiter.api.Test; - -import org.neo4j.driver.Config; -import org.neo4j.driver.Driver; -import org.neo4j.driver.GraphDatabase; -import org.neo4j.driver.exceptions.UntrustedServerException; -import org.neo4j.driver.util.StubServer; - -import static org.hamcrest.core.IsEqual.equalTo; -import static org.hamcrest.junit.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.neo4j.driver.Logging.none; - -class TrustedServerProductTest -{ - private static final Config config = Config.builder() - .withoutEncryption() - .withLogging( none() ) - .build(); - - @Test - void shouldRejectConnectionsToNonNeo4jServers() throws Exception - { - StubServer server = StubServer.start( "untrusted_server.script", 9001 ); - final Driver driver = GraphDatabase.driver( "bolt://127.0.0.1:9001", config ); - assertThrows( UntrustedServerException.class, driver::verifyConnectivity ); - assertThat( server.exitStatus(), equalTo( 0 ) ); - } - -} diff --git a/driver/src/test/resources/acquire_endpoints_v3.script b/driver/src/test/resources/acquire_endpoints_v3.script deleted file mode 100644 index a829e7a780..0000000000 --- a/driver/src/test/resources/acquire_endpoints_v3.script +++ /dev/null @@ -1,10 +0,0 @@ -!: BOLT 3 -!: AUTO RESET -!: AUTO HELLO -!: AUTO GOODBYE - -C: RUN "CALL dbms.cluster.routing.getRoutingTable($context)" {"context": {"address": "127.0.0.1:9001"}} {} - PULL_ALL -S: SUCCESS {"fields": ["ttl", "servers"]} - RECORD [9223372036854775807, [{"addresses": ["127.0.0.1:9007","127.0.0.1:9008"],"role": "WRITE"}, {"addresses": ["127.0.0.1:9005","127.0.0.1:9006"], "role": "READ"},{"addresses": ["127.0.0.1:9004","127.0.0.1:9002","127.0.0.1:9003"], "role": "ROUTE"}]] - SUCCESS {} diff --git a/driver/src/test/resources/dead_read_server_tx.script b/driver/src/test/resources/dead_read_server_tx.script deleted file mode 100644 index 0451d49776..0000000000 --- a/driver/src/test/resources/dead_read_server_tx.script +++ /dev/null @@ -1,9 +0,0 @@ -!: BOLT 3 -!: AUTO RESET -!: AUTO HELLO -!: AUTO GOODBYE -!: AUTO BEGIN - -C: RUN "MATCH (n) RETURN n.name" {} {} -C: PULL_ALL -S: diff --git a/driver/src/test/resources/dead_write_server.script b/driver/src/test/resources/dead_write_server.script deleted file mode 100644 index d8af7ce97e..0000000000 --- a/driver/src/test/resources/dead_write_server.script +++ /dev/null @@ -1,9 +0,0 @@ -!: BOLT 3 -!: AUTO RESET -!: AUTO HELLO -!: AUTO GOODBYE -!: AUTO BEGIN - -C: RUN "CREATE (n {name:'Bob'})" {} {} -C: PULL_ALL -S: diff --git a/driver/src/test/resources/read_server_v3_read.script b/driver/src/test/resources/read_server_v3_read.script deleted file mode 100644 index 04271fc8c5..0000000000 --- a/driver/src/test/resources/read_server_v3_read.script +++ /dev/null @@ -1,12 +0,0 @@ -!: BOLT 3 -!: AUTO RESET -!: AUTO HELLO -!: AUTO GOODBYE - -C: RUN "MATCH (n) RETURN n.name" {} { "mode": "r" } - PULL_ALL -S: SUCCESS {"fields": ["n.name"]} - RECORD ["Bob"] - RECORD ["Alice"] - RECORD ["Tina"] - SUCCESS {} diff --git a/driver/src/test/resources/reset_error.script b/driver/src/test/resources/reset_error.script deleted file mode 100644 index 715e80d670..0000000000 --- a/driver/src/test/resources/reset_error.script +++ /dev/null @@ -1,12 +0,0 @@ -!: BOLT 3 -!: AUTO HELLO -!: AUTO GOODBYE - -C: RUN "RETURN 42 AS answer" {} {} - PULL_ALL -S: SUCCESS {"fields": ["answer"]} - RECORD [42] - SUCCESS {} -C: RESET -S: FAILURE {"code": "Neo.TransientError.General.DatabaseUnavailable", "message": "Unable to reset"} - diff --git a/driver/src/test/resources/untrusted_server.script b/driver/src/test/resources/untrusted_server.script deleted file mode 100644 index ee7ced5708..0000000000 --- a/driver/src/test/resources/untrusted_server.script +++ /dev/null @@ -1,6 +0,0 @@ -!: BOLT 3 -!: AUTO RESET -!: AUTO GOODBYE - -C: HELLO {"scheme": "none", "user_agent": "neo4j-java/dev"} -S: SUCCESS {"server": "AgentSmith/0.0.1", "connection_id": "bolt-123456789"} diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/CommandProcessor.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/CommandProcessor.java index ca752be049..de35a37305 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/CommandProcessor.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/CommandProcessor.java @@ -33,6 +33,8 @@ import java.io.UncheckedIOException; import org.neo4j.driver.exceptions.Neo4jException; +import org.neo4j.driver.exceptions.UntrustedServerException; +import org.neo4j.driver.internal.async.pool.ConnectionPoolImpl; public class CommandProcessor { @@ -149,6 +151,20 @@ else if ( currentLine.equals( "#request end" ) ) writeResponse( driverError( id, (Neo4jException) e ) ); System.out.println( "Neo4jException: " + e ); } + else if ( isConnectionPoolClosedException( e ) || e instanceof UntrustedServerException ) + { + String id = testkitState.newId(); + DriverError driverError = DriverError.builder() + .data( + DriverError.DriverErrorBody.builder() + .id( id ) + .errorType( e.getClass().getName() ) + .msg( e.getMessage() ) + .build() + ) + .build(); + writeResponse( driverError ); + } else { // Unknown error, interpret this as a backend error. @@ -219,4 +235,10 @@ private void writeResponse( TestkitResponse response ) throw new RuntimeException( "Error writing response", ex ); } } + + private boolean isConnectionPoolClosedException( Exception e ) + { + return e instanceof IllegalStateException && e.getMessage() != null && + e.getMessage().equals( ConnectionPoolImpl.CONNECTION_POOL_CLOSED_ERROR_MESSAGE ); + } } diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java index 03600fadbe..211f07f665 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/GetFeatures.java @@ -39,7 +39,8 @@ public class GetFeatures implements TestkitRequest "Optimization:PullPipelining", "ConfHint:connection.recv_timeout_seconds", "Temporary:TransactionClose", - "Temporary:DriverFetchSize" + "Temporary:DriverFetchSize", + "Temporary:DriverMaxTxRetryTime" ) ); @Override diff --git a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java index 9821d59c21..14bca56974 100644 --- a/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java +++ b/testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/NewDriver.java @@ -90,10 +90,13 @@ public TestkitResponse process( TestkitState testkitState ) Optional.ofNullable( data.userAgent ).ifPresent( configBuilder::withUserAgent ); Optional.ofNullable( data.connectionTimeoutMs ).ifPresent( timeout -> configBuilder.withConnectionTimeout( timeout, TimeUnit.MILLISECONDS ) ); Optional.ofNullable( data.fetchSize ).ifPresent( configBuilder::withFetchSize ); + RetrySettings retrySettings = Optional.ofNullable( data.maxTxRetryTimeMs ) + .map( RetrySettings::new ) + .orElse( RetrySettings.DEFAULT ); org.neo4j.driver.Driver driver; try { - driver = driver( URI.create( data.uri ), authToken, configBuilder.build(), domainNameResolver, testkitState, id ); + driver = driver( URI.create( data.uri ), authToken, configBuilder.build(), retrySettings, domainNameResolver, testkitState, id ); } catch ( RuntimeException e ) { @@ -143,11 +146,11 @@ private DomainNameResolver callbackDomainNameResolver( TestkitState testkitState }; } - private org.neo4j.driver.Driver driver( URI uri, AuthToken authToken, Config config, DomainNameResolver domainNameResolver, TestkitState testkitState, + private org.neo4j.driver.Driver driver( URI uri, AuthToken authToken, Config config, RetrySettings retrySettings, DomainNameResolver domainNameResolver, + TestkitState testkitState, String driverId ) { RoutingSettings routingSettings = RoutingSettings.DEFAULT; - RetrySettings retrySettings = RetrySettings.DEFAULT; SecuritySettings.SecuritySettingsBuilder securitySettingsBuilder = new SecuritySettings.SecuritySettingsBuilder(); SecuritySettings securitySettings = securitySettingsBuilder.build(); SecurityPlan securityPlan = securitySettings.createSecurityPlan( uri.getScheme() ); @@ -181,6 +184,7 @@ public static class NewDriverBody private boolean domainNameResolverRegistered; private Long connectionTimeoutMs; private Integer fetchSize; + private Long maxTxRetryTimeMs; } @RequiredArgsConstructor