Skip to content

Commit

Permalink
Allow tx timeout to be 0 or null. (#1108)
Browse files Browse the repository at this point in the history 
* Allow tx timeout to be 0 or null.

Adjust TestKit back end to accept
 * `null` timeout: send `null` to server (or omit as it's the default)
 * and integer: configure as timeout in milliseconds
 * omitted timeout: don't explicitly configure the timeout (user driver default)

* Adjust unit tests

* Add header

* Code review suggestions by Dmitriy

Co-authored-by: Dmitriy Tverdiakov <dmitriy.tverdiakov@neo4j.com>

* Changed to Tx config builter `.withDefaultTimeout` + clean-up

Co-authored-by: Dmitriy Tverdiakov <dmitriy.tverdiakov@neo4j.com>

* Fix typos

Co-authored-by: Dmitriy Tverdiakov <dmitriy.tverdiakov@neo4j.com>
  • Loading branch information
@robsdedude @injectives
robsdedude and injectives authored Jan 18, 2022
1 parent 868f846 commit 933fc4e
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 34 deletions.
17 changes: 15 additions & 2 deletions driver/src/main/java/org/neo4j/driver/TransactionConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,25 +189,38 @@ private Builder()

/**
* Set the transaction timeout. Transactions that execute longer than the configured timeout will be terminated by the database.
* See also {@link #withDefaultTimeout}.
* <p>
* This functionality allows to limit query/transaction execution time. Specified timeout overrides the default timeout configured in the database
* using {@code dbms.transaction.timeout} setting.
* <p>
* Provided value should not be {@code null} and should not represent a duration of zero or negative duration.
* Provided value should not represent a negative duration.
*
* @param timeout the timeout.
* @return this builder.
*/
public Builder withTimeout( Duration timeout )
{
requireNonNull( timeout, "Transaction timeout should not be null" );
checkArgument( !timeout.isZero(), "Transaction timeout should not be zero" );
checkArgument( !timeout.isNegative(), "Transaction timeout should not be negative" );

this.timeout = timeout;
return this;
}

/**
* Set the transaction timeout to the server-side configured default timeout. This is the default behaviour if
* {@link #withTimeout} has not been called.
* See also {@link #withTimeout}.
*
* @return this builder.
*/
public Builder withDefaultTimeout()
{
this.timeout = null;
return this;
}

/**
* Set the transaction metadata. Specified metadata will be attached to the executing transaction and visible in the output of
* {@code dbms.listQueries} and {@code dbms.listTransactions} procedures. It will also get logged to the {@code query.log}.
Expand Down
39 changes: 24 additions & 15 deletions driver/src/test/java/org/neo4j/driver/TransactionConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@
import java.util.HashMap;
import java.util.Map;

import org.neo4j.driver.exceptions.ClientException;
import org.neo4j.driver.internal.InternalNode;
import org.neo4j.driver.internal.InternalPath;
import org.neo4j.driver.internal.InternalRelationship;
import org.neo4j.driver.exceptions.ClientException;
import org.neo4j.driver.util.TestUtil;

import static java.util.Collections.emptyMap;
Expand All @@ -51,18 +51,6 @@ void emptyConfigShouldHaveNoMetadata()
assertEquals( emptyMap(), TransactionConfig.empty().metadata() );
}

@Test
void shouldDisallowNullTimeout()
{
assertThrows( NullPointerException.class, () -> TransactionConfig.builder().withTimeout( null ) );
}

@Test
void shouldDisallowZeroTimeout()
{
assertThrows( IllegalArgumentException.class, () -> TransactionConfig.builder().withTimeout( Duration.ZERO ) );
}

@Test
void shouldDisallowNegativeTimeout()
{
Expand Down Expand Up @@ -92,12 +80,33 @@ void shouldDisallowMetadataWithIllegalValues()
void shouldHaveTimeout()
{
TransactionConfig config = TransactionConfig.builder()
.withTimeout( Duration.ofSeconds( 3 ) )
.build();
.withTimeout( Duration.ofSeconds( 3 ) )
.build();

assertEquals( Duration.ofSeconds( 3 ), config.timeout() );
}

@Test
void shouldAllowDefaultTimeout()
{
TransactionConfig config = TransactionConfig.builder()
.withTimeout( Duration.ofSeconds( 3 ) )
.withDefaultTimeout()
.build();

assertNull( config.timeout() );
}

@Test
void shouldAllowZeroTimeout()
{
TransactionConfig config = TransactionConfig.builder()
.withTimeout( Duration.ZERO )
.build();

assertEquals( Duration.ZERO, config.timeout() );
}

@Test
void shouldHaveMetadata()
{
Expand Down
27 changes: 27 additions & 0 deletions testkit-backend/src/main/java/neo4j/org/testkit/backend/CustomDriverError.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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 neo4j.org.testkit.backend;

public class CustomDriverError extends RuntimeException
{
public CustomDriverError( Throwable cause )
{
super( cause );
}
}
15 changes: 15 additions & 0 deletions ...c/main/java/neo4j/org/testkit/backend/channel/handler/TestkitRequestProcessorHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import io.netty.channel.Channel;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelInboundHandlerAdapter;
import neo4j.org.testkit.backend.CustomDriverError;
import neo4j.org.testkit.backend.TestkitState;
import neo4j.org.testkit.backend.messages.requests.TestkitRequest;
import neo4j.org.testkit.backend.messages.responses.BackendError;
Expand Down Expand Up @@ -145,6 +146,20 @@ else if ( isConnectionPoolClosedException( throwable ) || throwable instanceof U
)
.build();
}
else if ( throwable instanceof CustomDriverError )
{
throwable = throwable.getCause();
String id = testkitState.newId();
return DriverError.builder()
.data(
DriverError.DriverErrorBody.builder()
.id( id )
.errorType( throwable.getClass().getName() )
.msg( throwable.getMessage() )
.build()
)
.build();
}
else
{
return BackendError.builder().data( BackendError.BackendErrorBody.builder().msg( throwable.toString() ).build() ).build();
Expand Down
45 changes: 33 additions & 12 deletions ...nd/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionBeginTransaction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import lombok.Getter;
import lombok.Setter;
import neo4j.org.testkit.backend.CustomDriverError;
import neo4j.org.testkit.backend.TestkitState;
import neo4j.org.testkit.backend.holder.AsyncTransactionHolder;
import neo4j.org.testkit.backend.holder.RxTransactionHolder;
Expand All @@ -45,6 +46,28 @@ public class SessionBeginTransaction implements TestkitRequest
{
private SessionBeginTransactionBody data;

private void configureTimeout( TransactionConfig.Builder builder )
{
if ( data.getTimeoutPresent() )
{
try
{
if ( data.getTimeout() != null )
{
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
}
else
{
builder.withDefaultTimeout();
}
}
catch ( IllegalArgumentException e )
{
throw new CustomDriverError( e );
}
}
}

@Override
public TestkitResponse process( TestkitState testkitState )
{
Expand All @@ -53,10 +76,7 @@ public TestkitResponse process( TestkitState testkitState )
TransactionConfig.Builder builder = TransactionConfig.builder();
Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata );

if ( data.getTimeout() != null )
{
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
}
configureTimeout( builder );

org.neo4j.driver.Transaction transaction = session.beginTransaction( builder.build() );
return transaction( testkitState.addTransactionHolder( new TransactionHolder( sessionHolder, transaction ) ) );
Expand All @@ -72,10 +92,7 @@ public CompletionStage<TestkitResponse> processAsync( TestkitState testkitState
TransactionConfig.Builder builder = TransactionConfig.builder();
Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata );

if ( data.getTimeout() != null )
{
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
}
configureTimeout( builder );

return session.beginTransactionAsync( builder.build() ).thenApply( tx -> transaction(
testkitState.addAsyncTransactionHolder( new AsyncTransactionHolder( sessionHolder, tx ) ) ) );
Expand All @@ -92,10 +109,7 @@ public Mono<TestkitResponse> processRx( TestkitState testkitState )
TransactionConfig.Builder builder = TransactionConfig.builder();
Optional.ofNullable( data.txMeta ).ifPresent( builder::withMetadata );

if ( data.getTimeout() != null )
{
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
}
configureTimeout( builder );

return Mono.fromDirect( session.beginTransaction( builder.build() ) )
.map( tx -> transaction(
Expand All @@ -115,5 +129,12 @@ public static class SessionBeginTransactionBody
private String sessionId;
private Map<String,Object> txMeta;
private Integer timeout;
private Boolean timeoutPresent = false;

public void setTimeout( Integer timeout )
{
this.timeout = timeout;
timeoutPresent = true;
}
}
}
37 changes: 32 additions & 5 deletions testkit-backend/src/main/java/neo4j/org/testkit/backend/messages/requests/SessionRun.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import lombok.Getter;
import lombok.Setter;
import neo4j.org.testkit.backend.CustomDriverError;
import neo4j.org.testkit.backend.TestkitState;
import neo4j.org.testkit.backend.holder.ResultCursorHolder;
import neo4j.org.testkit.backend.holder.ResultHolder;
Expand Down Expand Up @@ -50,6 +51,28 @@ public class SessionRun implements TestkitRequest
{
private SessionRunBody data;

private void configureTimeout( TransactionConfig.Builder builder )
{
if ( data.getTimeoutPresent() )
{
try
{
if ( data.getTimeout() != null )
{
builder.withTimeout( Duration.ofMillis( data.getTimeout() ) );
}
else
{
builder.withDefaultTimeout();
}
}
catch ( IllegalArgumentException e )
{
throw new CustomDriverError( e );
}
}
}

@Override
public TestkitResponse process( TestkitState testkitState )
{
Expand All @@ -60,7 +83,7 @@ public TestkitResponse process( TestkitState testkitState )
.orElseGet( () -> new Query( data.cypher ) );
TransactionConfig.Builder transactionConfig = TransactionConfig.builder();
Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata );
Optional.ofNullable( data.getTimeout() ).ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) );
configureTimeout( transactionConfig );
org.neo4j.driver.Result result = session.run( query, transactionConfig.build() );
String id = testkitState.addResultHolder( new ResultHolder( sessionHolder, result ) );

Expand All @@ -79,8 +102,7 @@ public CompletionStage<TestkitResponse> processAsync( TestkitState testkitState
.orElseGet( () -> new Query( data.cypher ) );
TransactionConfig.Builder transactionConfig = TransactionConfig.builder();
Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata );
Optional.ofNullable( data.getTimeout() )
.ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) );
configureTimeout( transactionConfig );

return session.runAsync( query, transactionConfig.build() )
.thenApply( resultCursor ->
Expand All @@ -104,7 +126,7 @@ public Mono<TestkitResponse> processRx( TestkitState testkitState )
.orElseGet( () -> new Query( data.cypher ) );
TransactionConfig.Builder transactionConfig = TransactionConfig.builder();
Optional.ofNullable( data.getTxMeta() ).ifPresent( transactionConfig::withMetadata );
Optional.ofNullable( data.getTimeout() ).ifPresent( to -> transactionConfig.withTimeout( Duration.ofMillis( to ) ) );
configureTimeout( transactionConfig );

RxResult result = session.run( query, transactionConfig.build() );
String id = testkitState.addRxResultHolder( new RxResultHolder( sessionHolder, result ) );
Expand All @@ -127,11 +149,16 @@ public static class SessionRunBody
{
@JsonDeserialize( using = TestkitCypherParamDeserializer.class )
private Map<String,Object> params;

private String sessionId;
private String cypher;
private Map<String,Object> txMeta;
private Integer timeout;
private Boolean timeoutPresent = false;

public void setTimeout( Integer timeout )
{
this.timeout = timeout;
timeoutPresent = true;
}
}
}

0 comments on commit 933fc4e

Please sign in to comment.