Merge pull request #468 from lutovich/1.4-revert-timeout

Revert "Use connect timeout in Bolt and TLS handshake"

Author: lutovich

driver/src/main/java/org/neo4j/driver/internal/net/ChannelFactory.java

@@ -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 )
             {
@@ -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 );

driver/src/main/java/org/neo4j/driver/internal/net/SocketClient.java

@@ -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;
@@ -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 );
@@ -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 )
@@ -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;
-    }
 }

driver/src/main/java/org/neo4j/driver/internal/net/UnencryptedSocketChannel.java

@@ -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;
@@ -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
@@ -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
     {