Skip to content

Revert "Use connect timeout in Bolt and TLS handshake" #468

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 26, 2018
Merged

Revert "Use connect timeout in Bolt and TLS handshake" #468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 13 additions & 6 deletions driver/src/main/java/org/neo4j/driver/internal/net/ChannelFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,31 @@
import java.net.ConnectException;
import java.net.Socket;
import java.net.SocketTimeoutException;
import java.net.StandardSocketOptions;
import java.nio.channels.ByteChannel;
import java.nio.channels.SocketChannel;

import org.neo4j.driver.internal.security.SecurityPlan;
import org.neo4j.driver.internal.security.TLSSocketChannel;
import org.neo4j.driver.v1.Logger;

class ChannelFactory
{
static ByteChannel create( Socket socket, BoltServerAddress address, SecurityPlan securityPlan, int timeoutMillis,
Logger log ) throws IOException
static ByteChannel create( BoltServerAddress address, SecurityPlan securityPlan, int timeoutMillis, Logger log )
throws IOException
{
connect( socket, address, timeoutMillis );
SocketChannel soChannel = SocketChannel.open();
soChannel.setOption( StandardSocketOptions.SO_REUSEADDR, true );
soChannel.setOption( StandardSocketOptions.SO_KEEPALIVE, true );
connect( soChannel, address, timeoutMillis );

ByteChannel channel = new UnencryptedSocketChannel( socket );
ByteChannel channel = soChannel;

if ( securityPlan.requiresEncryption() )
{
try
{
channel = TLSSocketChannel.create( address, securityPlan, channel, log );
channel = TLSSocketChannel.create( address, securityPlan, soChannel, log );
}
catch ( Exception e )
{
Expand All @@ -65,8 +70,10 @@ static ByteChannel create( Socket socket, BoltServerAddress address, SecurityPla
return channel;
}

private static void connect( Socket socket, BoltServerAddress address, int timeoutMillis ) throws IOException
private static void connect( SocketChannel soChannel, BoltServerAddress address, int timeoutMillis )
throws IOException
{
Socket socket = soChannel.socket();
try
{
socket.connect( address.toSocketAddress(), timeoutMillis );
Expand Down
62 changes: 2 additions & 60 deletions driver/src/main/java/org/neo4j/driver/internal/net/SocketClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,14 @@

import java.io.IOException;
import java.net.ConnectException;
import java.net.Socket;
import java.net.SocketAddress;
import java.net.SocketTimeoutException;
import java.nio.ByteBuffer;
import java.nio.channels.ByteChannel;
import java.util.Queue;

import org.neo4j.driver.internal.messaging.Message;
import org.neo4j.driver.internal.messaging.MessageFormat;
import org.neo4j.driver.internal.security.SecurityPlan;
import org.neo4j.driver.internal.security.TLSSocketChannel;
import org.neo4j.driver.internal.util.BytePrinter;
import org.neo4j.driver.v1.Config;
import org.neo4j.driver.v1.Logger;
import org.neo4j.driver.v1.exceptions.ClientException;
import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;
Expand Down Expand Up @@ -123,31 +118,22 @@ void blockingWrite( ByteBuffer buf ) throws IOException

public void start()
{
Socket socket = null;
boolean connected = false;
try
{
logger.debug( "Connecting to %s, secure: %s", address, securityPlan.requiresEncryption() );
if( channel == null )
{
socket = newSocket( timeoutMillis );
setChannel( ChannelFactory.create( socket, address, securityPlan, timeoutMillis, logger ) );
setChannel( ChannelFactory.create( address, securityPlan, timeoutMillis, logger ) );
logger.debug( "Connected to %s, secure: %s", address, securityPlan.requiresEncryption() );
}

logger.debug( "Negotiating protocol with %s", address );
SocketProtocol protocol = negotiateProtocol();
setProtocol( protocol );
logger.debug( "Selected protocol %s with %s", protocol.getClass(), address );

// reset read timeout (SO_TIMEOUT) to the original value of zero
// we do not want to permanently limit amount of time driver waits for database to execute query
socket.setSoTimeout( 0 );
connected = true;
}
catch ( ConnectException | SocketTimeoutException e )
catch ( ConnectException e )
{
// unable to connect socket or TLS/Bolt handshake took too much time
throw new ServiceUnavailableException( format(
"Unable to connect to %s, ensure the database is running and that there is a " +
"working network connection to it.", address ), e );
Expand All @@ -156,19 +142,6 @@ public void start()
{
throw new ServiceUnavailableException( "Unable to process request: " + e.getMessage(), e );
}
finally
{
if ( !connected && socket != null )
{
try
{
socket.close();
}
catch ( Throwable ignore )
{
}
}
}
}

public void updateProtocol( String serverVersion )
Expand Down Expand Up @@ -328,35 +301,4 @@ public BoltServerAddress address()
{
return address;
}

/**
* Creates new {@link Socket} object with {@link Socket#setSoTimeout(int) read timeout} set to the given value.
* Connection to bolt server includes:
* <ol>
* <li>TCP connect via {@link Socket#connect(SocketAddress, int)}</li>
* <li>Optional TLS handshake using {@link TLSSocketChannel}</li>
* <li>Bolt handshake</li>
* </ol>
* We do not want any of these steps to hang infinitely if server does not respond and thus:
* <ol>
* <li>Use {@link Socket#connect(SocketAddress, int)} with timeout, as configured in
* {@link Config#connectionTimeoutMillis()}</li>
* <li>Initially set {@link Socket#setSoTimeout(int) read timeout} on the socket. Same connection-timeout value
* from {@link Config#connectionTimeoutMillis()} is used. This way blocking reads during TLS and Bolt handshakes
* have limited waiting time</li>
* </ol>
*
* @param configuredConnectTimeout user-defined connection timeout to be initially used as read timeout.
* @return new socket.
* @throws IOException when creation or configuration of the socket fails.
*/
private static Socket newSocket( int configuredConnectTimeout ) throws IOException
{
Socket socket = new Socket();
socket.setReuseAddress( true );
socket.setKeepAlive( true );
// set read timeout initially
socket.setSoTimeout( configuredConnectTimeout );
return socket;
}
}
65 changes: 0 additions & 65 deletions driver/src/main/java/org/neo4j/driver/internal/net/UnencryptedSocketChannel.java
View file
Open in desktop

This file was deleted.

49 changes: 18 additions & 31 deletions driver/src/test/java/org/neo4j/driver/internal/net/SocketClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,24 @@
*/
package org.neo4j.driver.internal.net;

import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import java.io.IOException;
import java.net.ServerSocket;
import java.net.SocketTimeoutException;
import java.nio.ByteBuffer;
import java.nio.channels.ByteChannel;
import java.util.ArrayList;
import java.util.List;

import org.neo4j.driver.internal.security.SecurityPlan;
import org.neo4j.driver.v1.exceptions.ClientException;
import org.neo4j.driver.v1.exceptions.ServiceUnavailableException;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand All @@ -50,16 +49,24 @@ public class SocketClientTest
@Rule
public ExpectedException exception = ExpectedException.none();

// TODO: This is not possible with blocking NIO channels, unless we use inputStreams, but then we can't use
// off-heap buffers. We need to swap to use selectors, which would allow us to time out.
@Test
public void shouldFailWhenProtocolNegotiationTakesTooLong() throws Exception
@Ignore
public void testNetworkTimeout() throws Throwable
{
testReadTimeoutOnConnect( SecurityPlan.insecure() );
}
// Given a server that will never reply
ServerSocket server = new ServerSocket( 0 );
BoltServerAddress address = new BoltServerAddress( "localhost", server.getLocalPort() );

@Test
public void shouldFailWhenTLSHandshakeTakesTooLong() throws Exception
{
testReadTimeoutOnConnect( SecurityPlan.forAllCertificates() );
SocketClient client = dummyClient( address );

// Expect
exception.expect( ClientException.class );
exception.expectMessage( "database took longer than network timeout (100ms) to reply." );

// When
client.start();
}

@Test
Expand Down Expand Up @@ -183,26 +190,6 @@ public void shouldFailIfConnectionFailsWhileWriting() throws IOException
client.blockingWrite( buffer );
}

private static void testReadTimeoutOnConnect( SecurityPlan securityPlan ) throws IOException
{
try ( ServerSocket server = new ServerSocket( 0 ) ) // server that does not reply
{
int timeoutMillis = 1_000;
BoltServerAddress address = new BoltServerAddress( "localhost", server.getLocalPort() );
SocketClient client = new SocketClient( address, securityPlan, timeoutMillis, DEV_NULL_LOGGER );

try
{
client.start();
fail( "Exception expected" );
}
catch ( ServiceUnavailableException e )
{
assertThat( e.getCause(), instanceOf( SocketTimeoutException.class ) );
}
}
}

private static class ByteAtATimeChannel implements ByteChannel
{

Expand Down
Loading