Repository: netty/netty
Issue: Method setHandshakeTimeout of SslHandler does not work in server side
Labels: defect

### Expected behavior
I want to enable `setHandshakeTimeout()` in server side. I call the method setHandshakeTimeout() like this `sslHandler.setHandshakeTimeout(1,TimeUnit.MILLISECONDS);` The value of HandshakeTimeout is set to a very small value so that the client and server can not complete the handshake since the handshake is timeout. 

### Actual behavior
But it can not work when I use this in server.The connection can still be established between server and client. However when I do the same in client, the SSL connection can not be established , this is what I expect to see. 

### Steps to reproduce
1. call the `sslHandler.setHandshakeTimeoutMillis(1);` or `sslHandler.setHandshakeTimeout(1,TimeUnit.MILLISECONDS);` in server
2. do connect and SSL connection can be successfully established
3. do 1 and 2 again in client side, and SSL connection can not be established

### Minimal yet complete reproducer code (or URL to code)
You may use [SecureChat](http://netty.io/4.1/xref/io/netty/example/securechat/package-summary.html) example or any other SSL example to reproduce

### Netty version
4.1.15.Final

### JVM version (e.g. `java -version`)
java version "1.8.0_131"
Java(TM) SE Runtime Environment (build 1.8.0_131-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.131-b11, mixed mode)


Issue comments:
@BillyYccc please check https://github.com/netty/netty/pull/7277
Fixed

Pull request review comments:
![CRITICAL](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-critical.png) Remove this use of "Thread.sleep()". [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AS2925)

This is needed...
Swapping from `isDone()` to `await(10, MILLISECONDS)` is probably better.
@ejona86 actually this will not work as then we will try to call await() on the EventLoop thread which will then throw an exception. 

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
index ac32c9c76f0..f72cf874623 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -1536,11 +1536,15 @@ private void flush(ChannelHandlerContext ctx, ChannelPromise promise) throws Exc
     public void handlerAdded(final ChannelHandlerContext ctx) throws Exception {
         this.ctx = ctx;
 
-        if (ctx.channel().isActive() && engine.getUseClientMode()) {
-            // Begin the initial handshake.
-            // channelActive() event has been fired already, which means this.channelActive() will
-            // not be invoked. We have to initialize here instead.
-            handshake(null);
+        if (ctx.channel().isActive()) {
+            if (engine.getUseClientMode()) {
+                // Begin the initial handshake.
+                // channelActive() event has been fired already, which means this.channelActive() will
+                // not be invoked. We have to initialize here instead.
+                handshake(null);
+            } else {
+                applyHandshakeTimeout(null);
+            }
         } else {
             // channelActive() event has not been fired yet.  this.channelOpen() will be invoked
             // and initialization will occur there.
@@ -1635,17 +1639,21 @@ public void operationComplete(Future<Channel> future) throws Exception {
         } finally {
            forceFlush(ctx);
         }
+        applyHandshakeTimeout(p);
+    }
 
+    private void applyHandshakeTimeout(Promise<Channel> p) {
+        final Promise<Channel> promise = p == null ? handshakePromise : p;
         // Set timeout if necessary.
         final long handshakeTimeoutMillis = this.handshakeTimeoutMillis;
-        if (handshakeTimeoutMillis <= 0 || p.isDone()) {
+        if (handshakeTimeoutMillis <= 0 || promise.isDone()) {
             return;
         }
 
         final ScheduledFuture<?> timeoutFuture = ctx.executor().schedule(new Runnable() {
             @Override
             public void run() {
-                if (p.isDone()) {
+                if (promise.isDone()) {
                     return;
                 }
                 notifyHandshakeFailure(HANDSHAKE_TIMED_OUT);
@@ -1653,7 +1661,7 @@ public void run() {
         }, handshakeTimeoutMillis, TimeUnit.MILLISECONDS);
 
         // Cancel the handshake timeout when handshake is finished.
-        p.addListener(new FutureListener<Channel>() {
+        promise.addListener(new FutureListener<Channel>() {
             @Override
             public void operationComplete(Future<Channel> f) throws Exception {
                 timeoutFuture.cancel(false);
@@ -1671,9 +1679,13 @@ private void forceFlush(ChannelHandlerContext ctx) {
      */
     @Override
     public void channelActive(final ChannelHandlerContext ctx) throws Exception {
-        if (!startTls && engine.getUseClientMode()) {
-            // Begin the initial handshake
-            handshake(null);
+        if (!startTls) {
+            if (engine.getUseClientMode()) {
+                // Begin the initial handshake.
+                handshake(null);
+            } else {
+                applyHandshakeTimeout(null);
+            }
         }
         ctx.fireChannelActive();
     }


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
index 5e10a6d569b..478c77400f0 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -85,6 +85,36 @@
 
 public class SslHandlerTest {
 
+    @Test(expected = SSLException.class, timeout = 3000)
+    public void testClientHandshakeTimeout() throws Exception {
+        testHandshakeTimeout(true);
+    }
+
+    @Test(expected = SSLException.class, timeout = 3000)
+    public void testServerHandshakeTimeout() throws Exception {
+        testHandshakeTimeout(false);
+    }
+
+    private static void testHandshakeTimeout(boolean client) throws Exception {
+        SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(client);
+        SslHandler handler = new SslHandler(engine);
+        handler.setHandshakeTimeoutMillis(1);
+
+        EmbeddedChannel ch = new EmbeddedChannel(handler);
+        try {
+            while (!handler.handshakeFuture().isDone()) {
+                Thread.sleep(10);
+                // We need to run all pending tasks as the handshake timeout is scheduled on the EventLoop.
+                ch.runPendingTasks();
+            }
+
+            handler.handshakeFuture().syncUninterruptibly();
+        } finally {
+            ch.finishAndReleaseAll();
+        }
+    }
+
     @Test
     public void testTruncatedPacket() throws Exception {
         SSLEngine engine = SSLContext.getDefault().createSSLEngine();