This is an automated email from the ASF dual-hosted git repository.

gosullivan pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new b55215d  GEODE-3563: use a timeout for newly created sockets in 
TcpConduit.run() (#1671)
b55215d is described below

commit b55215dcb64c86554d531b0b475e85f013e76fc6
Author: Galen O'Sullivan <gosulli...@pivotal.io>
AuthorDate: Fri Mar 30 11:13:15 2018 -0700

    GEODE-3563: use a timeout for newly created sockets in TcpConduit.run() 
(#1671)
    
    * GEODE-3563: use a timeout in SocketCreator.ConfigureServerSSLSocket()
    
    Also close newly accepted sockets in TcpConduit.run() if SSL
    configuration fails (or any other IOException).
    
    * Add units.
    
    * Rename to startHandshakeIfSocketIsSSL for clarity.
---
 .../distributed/internal/tcpserver/TcpServer.java  |  5 +--
 .../internal/cache/tier/sockets/AcceptorImpl.java  |  3 +-
 .../apache/geode/internal/net/SocketCreator.java   |  7 +++-
 .../org/apache/geode/internal/tcp/TCPConduit.java  | 37 +++++++++++++---------
 .../geode/internal/net/DummySocketCreator.java     |  8 ++---
 .../internal/net/SSLSocketIntegrationTest.java     |  5 +--
 .../geode/internal/net/SocketCreatorJUnitTest.java | 17 ++++++++++
 7 files changed, 52 insertions(+), 30 deletions(-)

diff --git 
a/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
 
b/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
index 169b09d..778e3d9 100755
--- 
a/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
+++ 
b/geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
@@ -345,7 +345,6 @@ public class TcpServer {
       }
       handler.shutDown();
       synchronized (this) {
-        // this.shutDown = true;
         this.notifyAll();
       }
     }
@@ -360,9 +359,7 @@ public class TcpServer {
       long startTime = DistributionStats.getStatTime();
       DataInputStream input = null;
       try {
-
-        socket.setSoTimeout(READ_TIMEOUT);
-        getSocketCreator().configureServerSSLSocket(socket);
+        getSocketCreator().startHandshakeIfSocketIsSSL(socket, READ_TIMEOUT);
 
         try {
           input = new DataInputStream(socket.getInputStream());
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
 
b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
index d060ed2..83d3156 100755
--- 
a/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
@@ -1549,8 +1549,7 @@ public class AcceptorImpl implements Acceptor, Runnable, 
CommBufferPool {
   }
 
   private CommunicationMode getCommunicationModeForNonSelector(Socket socket) 
throws IOException {
-    socket.setSoTimeout(this.acceptTimeout);
-    this.socketCreator.configureServerSSLSocket(socket);
+    this.socketCreator.startHandshakeIfSocketIsSSL(socket, this.acceptTimeout);
     byte communicationModeByte = (byte) socket.getInputStream().read();
     if (communicationModeByte == -1) {
       throw new EOFException();
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java 
b/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
index 9478953..31456c0 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
@@ -968,8 +968,12 @@ public class SocketCreator {
 
   /**
    * Will be a server socket... this one simply registers the listeners.
+   *
+   * @param timeout the socket's timeout will be set to this (in milliseconds).
    */
-  public void configureServerSSLSocket(Socket socket) throws IOException {
+  public void startHandshakeIfSocketIsSSL(Socket socket, int timeout) throws 
IOException {
+    socket.setSoTimeout(timeout);
+
     if (socket instanceof SSLSocket) {
       SSLSocket sslSocket = (SSLSocket) socket;
       try {
@@ -990,6 +994,7 @@ public class SocketCreator {
               ex);
           throw ex;
         }
+        // else ignore
       }
     }
   }
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java 
b/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java
index 9483e08..5487039 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java
@@ -598,9 +598,8 @@ public class TCPConduit implements Runnable {
               
LocalizedStrings.TCPConduit_UNABLE_TO_SHUT_DOWN_LISTENER_WITHIN_0_MS_UNABLE_TO_INTERRUPT_SOCKET_ACCEPT_DUE_TO_JDK_BUG_GIVING_UP,
               Integer.valueOf(LISTENER_CLOSE_TIMEOUT)));
         }
-      } catch (IOException e) {
-      } catch (InterruptedException e) {
-        // Ignore, we're trying to stop already.
+      } catch (IOException | InterruptedException e) {
+        // we're already trying to shutdown, ignore
       } finally {
         this.hsPool.shutdownNow();
       }
@@ -689,7 +688,7 @@ public class TCPConduit implements Runnable {
                 ex);
             break;
           }
-          socketCreator.configureServerSSLSocket(othersock);
+          socketCreator.startHandshakeIfSocketIsSSL(othersock, 
idleConnectionTimeout);
         }
         if (stopped) {
           try {
@@ -705,11 +704,19 @@ public class TCPConduit implements Runnable {
 
       } catch (ClosedByInterruptException cbie) {
         // safe to ignore
-      } catch (ClosedChannelException e) {
-        break; // we're dead
-      } catch (CancelException e) {
+      } catch (ClosedChannelException | CancelException e) {
         break;
-      } catch (Exception e) {
+      } catch (IOException e) {
+        this.getStats().incFailedAccept();
+
+        try {
+          if (othersock != null) {
+            othersock.close();
+          }
+        } catch (IOException ignore) {
+
+        }
+
         if (!stopped) {
           if (e instanceof SocketException && "Socket 
closed".equalsIgnoreCase(e.getMessage())) {
             // safe to ignore; see bug 31156
@@ -737,17 +744,17 @@ public class TCPConduit implements Runnable {
                 }
               }
             }
+          } else if ("Too many open files".equals(e.getMessage())) {
+            getConTable().fileDescriptorsExhausted();
           } else {
-            this.getStats().incFailedAccept();
-            if (e instanceof IOException && "Too many open 
files".equals(e.getMessage())) {
-              getConTable().fileDescriptorsExhausted();
-            } else {
-              logger.warn(e.getMessage(), e);
-            }
+            logger.warn(e.getMessage(), e);
           }
+
         }
-        // connections.cleanupLowWater();
+      } catch (Exception e) {
+        logger.warn(e.getMessage(), e);
       }
+
       if (!stopped && socket.isClosed()) {
         // NOTE: do not check for distributed system closing here. Messaging
         // may need to occur during the closing of the DS or cache
diff --git 
a/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java
 
b/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java
index 6928b6f..e061f14 100644
--- 
a/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java
+++ 
b/geode-core/src/test/java/org/apache/geode/internal/net/DummySocketCreator.java
@@ -35,12 +35,8 @@ public class DummySocketCreator extends SocketCreator {
   }
 
   @Override
-  public void configureServerSSLSocket(Socket socket) throws IOException {
-    this.socketSoTimeouts.add(socket.getSoTimeout());
+  public void startHandshakeIfSocketIsSSL(Socket socket, int timeout) throws 
IOException {
+    this.socketSoTimeouts.add(timeout);
     throw new SSLException("This is a test SSLException");
   }
-
-  public List<Integer> getSocketSoTimeouts() {
-    return socketSoTimeouts;
-  }
 }
diff --git 
a/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
 
b/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
index 704beb6..af06241 100755
--- 
a/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
+++ 
b/geode-core/src/test/java/org/apache/geode/internal/net/SSLSocketIntegrationTest.java
@@ -209,7 +209,7 @@ public class SSLSocketIntegrationTest {
       Awaitility.await("connect to server socket").atMost(30, 
TimeUnit.SECONDS).until(() -> {
         try {
           Socket clientSocket = socketCreator.connectForClient(
-              SocketCreator.getLocalHost().getHostAddress(), serverSocketPort, 
2000);
+              SocketCreator.getLocalHost().getHostAddress(), serverSocketPort, 
500);
           clientSocket.close();
           System.err.println(
               "client successfully connected to server but should not have 
been able to do so");
@@ -254,7 +254,8 @@ public class SSLSocketIntegrationTest {
     Thread serverThread = new Thread(new 
MyThreadGroup(this.testName.getMethodName()), () -> {
       try {
         Socket socket = serverSocket.accept();
-        
SocketCreatorFactory.getSocketCreatorForComponent(CLUSTER).configureServerSSLSocket(socket);
+        SocketCreatorFactory.getSocketCreatorForComponent(CLUSTER)
+            .startHandshakeIfSocketIsSSL(socket, 15000);
         ObjectInputStream ois = new ObjectInputStream(socket.getInputStream());
         messageFromClient.set((String) ois.readObject());
       } catch (IOException | ClassNotFoundException e) {
diff --git 
a/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
 
b/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
index 2fcd7f6..33d34bb 100644
--- 
a/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
+++ 
b/geode-core/src/test/java/org/apache/geode/internal/net/SocketCreatorJUnitTest.java
@@ -14,6 +14,12 @@
  */
 package org.apache.geode.internal.net;
 
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+import java.net.Socket;
+
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
@@ -37,6 +43,17 @@ public class SocketCreatorJUnitTest {
 
   }
 
+  @Test
+  public void testConfigureServerSSLSocketSetsSoTimeout() throws Exception {
+    final SocketCreator socketCreator = new 
SocketCreator(mock(SSLConfig.class));
+    final Socket socket = mock(Socket.class);
+
+    final int timeout = 1938236;
+    socketCreator.startHandshakeIfSocketIsSSL(socket, timeout);
+
+    verify(socket).setSoTimeout(timeout);
+  }
+
   private String getSingleKeyKeystore() {
     return TestUtil.getResourcePath(getClass(), "/ssl/trusted.keystore");
   }

-- 
To stop receiving notification emails like this one, please contact
gosulli...@apache.org.

Reply via email to