Repository: netty/netty
Issue: Channel promise of a write is successful, although SSL handshake fails.
Labels: defect

### Expected behavior
When a SSL handshake fails, a channel promise of a write operation should fail aswell. 

### Actual behavior
The channel promise succeeds. 

### Steps to reproduce

    $ git clone https://github.com/s-gheldd/netty-reproducer.git
    $ cd netty-reproducer
    $ mvn test

### Minimal yet complete reproducer code (or URL to code)

https://github.com/s-gheldd/netty-reproducer

### Netty version

`4.1.16.Final`

(last version that had the expected behavior `4.1.13.Final`)

### JVM version (e.g. `java -version`)

    java version "1.7.0_80"
    Java(TM) SE Runtime Environment (build 1.7.0_80-b15)
    Java HotSpot(TM) 64-Bit Server VM (build 24.80-b11, mixed mode)

### OS version (e.g. `uname -a`)

    Darwin ghelds-MacBook-Pro.local 15.6.0 Darwin Kernel Version 15.6.0: Mon Oct  2 22:20:08 PDT 2017; root:xnu-3248.71.4~1/RELEASE_X86_64 x86_64


Issue comments:
See https://github.com/netty/netty/pull/7380

Pull request review comments:

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 4cd8013ba0a..82fc18657a4 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -794,7 +794,10 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
                     return;
                 } else {
                     if (buf.isReadable()) {
-                        pendingUnencryptedWrites.addFirst(buf);
+                        pendingUnencryptedWrites.addFirst(buf, promise);
+                        // When we add the buffer/promise pair back we need to be sure we don't complete the promise
+                        // later in finishWrap. We only complete the promise if the buffer is completely consumed.
+                        promise = null;
                     } else {
                         buf.release();
                     }
@@ -1515,9 +1518,13 @@ private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause, boo
             notifyHandshakeFailure(cause);
         } finally {
             // Ensure we remove and fail all pending writes in all cases and so release memory quickly.
-            if (pendingUnencryptedWrites != null) {
-                pendingUnencryptedWrites.releaseAndFailAll(ctx, cause);
-            }
+            releaseAndFailAll(cause);
+        }
+    }
+
+    private void releaseAndFailAll(Throwable cause) {
+        if (pendingUnencryptedWrites != null) {
+            pendingUnencryptedWrites.releaseAndFailAll(ctx, cause);
         }
     }
 
@@ -1701,7 +1708,11 @@ public void run() {
                 if (promise.isDone()) {
                     return;
                 }
-                notifyHandshakeFailure(HANDSHAKE_TIMED_OUT);
+                try {
+                    notifyHandshakeFailure(HANDSHAKE_TIMED_OUT);
+                } finally {
+                    releaseAndFailAll(HANDSHAKE_TIMED_OUT);
+                }
             }
         }, handshakeTimeoutMillis, TimeUnit.MILLISECONDS);
 
diff --git a/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java b/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java
index 414847e894f..de9839fa61e 100644
--- a/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java
+++ b/transport/src/main/java/io/netty/channel/AbstractCoalescingBufferQueue.java
@@ -50,10 +50,11 @@ protected AbstractCoalescingBufferQueue(Channel channel, int initSize) {
     /**
      * Add a buffer to the front of the queue.
      */
-    public final void addFirst(ByteBuf buf) {
+    public final void addFirst(ByteBuf buf, ChannelPromise promise) {
         // Listener would be added here, but since it is null there is no need. The assumption is there is already a
         // listener at the front of the queue, or there is a buffer at the front of the queue, which was spliced from
         // buf via remove().
+        bufAndListenerPairs.addFirst(new DelegatingChannelPromiseNotifier(promise));
         bufAndListenerPairs.addFirst(buf);
 
         incrementReadableBytes(buf.readableBytes());


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 1e78a6e47dd..0afe89ccf73 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
@@ -30,6 +30,7 @@
 import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.ChannelOutboundHandlerAdapter;
+import io.netty.channel.ChannelPromise;
 import io.netty.channel.EventLoopGroup;
 import io.netty.channel.SimpleChannelInboundHandler;
 import io.netty.channel.embedded.EmbeddedChannel;
@@ -57,6 +58,7 @@
 import java.net.InetSocketAddress;
 import java.nio.channels.ClosedChannelException;
 import java.security.KeyStore;
+import java.security.NoSuchAlgorithmException;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
 import java.util.concurrent.BlockingQueue;
@@ -185,6 +187,22 @@ public void testNonByteBufNotPassThrough() throws Exception {
         }
     }
 
+    @Test
+    public void testIncompleteWriteDoesNotCompletePromisePrematurely() throws NoSuchAlgorithmException {
+        SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(false);
+
+        EmbeddedChannel ch = new EmbeddedChannel(new SslHandler(engine));
+
+        ChannelPromise promise = ch.newPromise();
+        ByteBuf buf = Unpooled.buffer(10).writeZero(10);
+        ch.writeAndFlush(buf, promise);
+        assertFalse(promise.isDone());
+        assertTrue(ch.finishAndReleaseAll());
+        assertTrue(promise.isDone());
+        assertThat(promise.cause(), is(instanceOf(SSLException.class)));
+    }
+
     @Test
     public void testReleaseSslEngine() throws Exception {
         assumeTrue(OpenSsl.isAvailable());