Repository: netty/netty
Issue: VoidChannelPromise eats exceptions...
Labels: defect

### Expected behavior

```
voidPromise
ChannelPromise voidPromise()
Return a special ChannelPromise which can be reused for different operations.
It's only supported to use it for write(Object, ChannelPromise).

Be aware that the returned ChannelPromise will not support most operations and should only be used if you want to save an object allocation for every write operation. 
You will not be able to detect if the operation was complete, only if it failed as the implementation will call ChannelPipeline.fireExceptionCaught(Throwable) in this case.
```

when writing a packet with a void promise, an exception should be thrown on failure.

### Actual behavior
the exception is eaten and it's not forwarded to the channel pipeline

### Steps to reproduce

### Minimal yet complete reproducer code (or URL to code)
This is CLEARLY a Bug.
in this function https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java#L736
when exception is thrown the logic goes to here
https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java#L834
and...the exception is ignored.
If the promise is void, it should be forwarded to the pipeline as stated in the API...

cc @Scottmitch @normanmaurer 
### Netty version
4.1.9-FINAL
### JVM version (e.g. `java -version`)

### OS version (e.g. `uname -a`)


Issue comments:
```patch

From 7beda8e105e05eb3b3c7e0c6b2f99a069370137f Mon Sep 17 00:00:00 2001
Date: Tue, 11 Apr 2017 17:34:36 +0200
Subject: [PATCH] Fix encoder exceptions being discarded when writing with a VoidPromise.

Motivation:

According to documentation of Channel#voidPromise(), if encoding of the message throws an exception, exception should be passed to the channel pipeline.
Unfortunately, that's not the case, which this commit fixes.
I've been ripping my hair out for hours trying to find out a memory leak without any clue.
There was an exception thrown in my handler which made the reason very obvious, but it was being silently discarded.
Fixes #6622

Modifications:

Fix the logic of notifyOutboundHandler to handle a case when the promise is not void.

Result:

Encoder exceptions will be properly propagated to the pipeline when used with void promise, in accordance to javadocs of Channel#voidPromise()
---
 transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java | 2 ++
 1 file changed, 2 insertions(+), 0 deletions(-)

diff --git a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
index 55e596f..d138307 100644
--- a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
+++ b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
@@ -834,6 +834,8 @@ abstract class AbstractChannelHandlerContext extends DefaultAttributeMap
     private static void notifyOutboundHandlerException(Throwable cause, ChannelPromise promise) {
         if (!(promise instanceof VoidChannelPromise)) {
             PromiseNotificationUtil.tryFailure(promise, cause, logger);
+        } else {
+            invokeExceptionCaught(t);
         }
     }
 
--
libgit2 0.24.0
```
@normanmaurer could you take a look at this patch :)
@ninja- yes this is clearly a bug... Your fix is not 100 % correct as it not handles all the cases tho. Please check https://github.com/netty/netty/pull/6634 for a full fix.  Thanks for reporting!
Thanks! Would you mind explaining which case I would miss with my version of the fix :)?
@ninja- the problem in the ChannelOutboundBuffer and also it would not start at the head of the pipeline
I've been pretty confident that I chose the function that starts propagation at the head...But OK thanks :)
Fixed by https://github.com/netty/netty/pull/6634

Pull request review comments:
How about moving `promise instanceof VoidChannelPromise` into `PromiseNotificationUtil`?

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
index 55e596f7f6b..796bddf0fff 100644
--- a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
+++ b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
@@ -832,9 +832,9 @@ public ChannelFuture writeAndFlush(Object msg) {
     }
 
     private static void notifyOutboundHandlerException(Throwable cause, ChannelPromise promise) {
-        if (!(promise instanceof VoidChannelPromise)) {
-            PromiseNotificationUtil.tryFailure(promise, cause, logger);
-        }
+        // Only log if the given promise is not of type VoidChannelPromise as tryFailure(...) is expected to return
+        // false.
+        PromiseNotificationUtil.tryFailure(promise, cause, promise instanceof VoidChannelPromise ? null : logger);
     }
 
     private void notifyHandlerException(Throwable cause) {
diff --git a/transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java b/transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java
index 52bf57d8dac..34fbde3556d 100644
--- a/transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java
+++ b/transport/src/main/java/io/netty/channel/ChannelOutboundBuffer.java
@@ -664,15 +664,15 @@ public void run() {
     }
 
     private static void safeSuccess(ChannelPromise promise) {
-        if (!(promise instanceof VoidChannelPromise)) {
-            PromiseNotificationUtil.trySuccess(promise, null, logger);
-        }
+        // Only log if the given promise is not of type VoidChannelPromise as trySuccess(...) is expected to return
+        // false.
+        PromiseNotificationUtil.trySuccess(promise, null, promise instanceof VoidChannelPromise ? null : logger);
     }
 
     private static void safeFail(ChannelPromise promise, Throwable cause) {
-        if (!(promise instanceof VoidChannelPromise)) {
-            PromiseNotificationUtil.tryFailure(promise, cause, logger);
-        }
+        // Only log if the given promise is not of type VoidChannelPromise as tryFailure(...) is expected to return
+        // false.
+        PromiseNotificationUtil.tryFailure(promise, cause, promise instanceof VoidChannelPromise ? null : logger);
     }
 
     @Deprecated


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
index 4f745e439cf..39f6752b2da 100644
--- a/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
+++ b/transport/src/test/java/io/netty/channel/DefaultChannelPipelineTest.java
@@ -1073,6 +1073,35 @@ public void testNotPinExecutor() {
         group.shutdownGracefully(0, 0, TimeUnit.SECONDS);
     }
 
+    @Test(timeout = 3000)
+    public void testVoidPromiseNotify() throws Throwable {
+        ChannelPipeline pipeline1 = new LocalChannel().pipeline();
+
+        EventLoopGroup defaultGroup = new DefaultEventLoopGroup(1);
+        EventLoop eventLoop1 = defaultGroup.next();
+        final Promise<Throwable> promise = eventLoop1.newPromise();
+        final Exception exception = new IllegalArgumentException();
+        try {
+            eventLoop1.register(pipeline1.channel()).syncUninterruptibly();
+            pipeline1.addLast(new ChannelDuplexHandler() {
+                @Override
+                public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) throws Exception {
+                    throw exception;
+                }
+
+                @Override
+                public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+                    promise.setSuccess(cause);
+                }
+            });
+            pipeline1.write("test", pipeline1.voidPromise());
+            assertSame(exception, promise.syncUninterruptibly().getNow());
+        } finally {
+            pipeline1.channel().close().syncUninterruptibly();
+            defaultGroup.shutdownGracefully();
+        }
+    }
+
     private static final class TestTask implements Runnable {
 
         private final ChannelPipeline pipeline;