Repository: netty/netty
Issue: EmbeddedChannel Close Regression with 4.1.35.Final
Labels: defect
regression

In one of my unit tests I have code like:

```java
    final Throwable expectedCause = new Exception("something failed");
    EmbeddedChannel channel = new EmbeddedChannel(new ChannelOutboundHandlerAdapter() {
      @Override
      public void close(ChannelHandlerContext ctx, ChannelPromise promise) {
        promise.tryFailure(expectedCause);
      }
    });
   channel.disconnect().addListener(future -> {
    // future should fail, but is successful since 4.1.35.Final
   });
```

To simulate a failing channel close. This worked great up until 4.1.34.Final, but in the latest version 4.1.35.Final this test failed. 

Turns out when I debug into it, the close method of the `ChannelOutboundHandlerAdapter` is never called on `channel.disconnect()` and as a result my assertions fail since the channel close completes successfully instead of triggering the failure.

Is this expected?

Issue comments:
Nope will look into it next week.

Does it work when calling channel.close() ?

> Am 26.04.2019 um 14:40 schrieb Michael Nitschinger <notifications@github.com>:
> 
> In one of my unit tests I have code like:
> 
>     final Throwable expectedCause = new Exception("something failed");
>     EmbeddedChannel channel = new EmbeddedChannel(new ChannelOutboundHandlerAdapter() {
>       @Override
>       public void close(ChannelHandlerContext ctx, ChannelPromise promise) {
>         promise.tryFailure(expectedCause);
>       }
>     });
> To simulate a failing channel close. This worked great up until 4.1.34.Final, but in the latest version 4.1.35.Final this test failed.
> 
> Turns out when I debug into it, the close method of the ChannelOutboundHandlerAdapter is never called on channel.disconnect() and as a result my assertions fail since the channel close completes successfully instead of triggering the failure.
> 
> Is this expected?
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.

Yes, when calling `channel.close()` my handler is triggered, when calling `channel.disconnect()` it is not.
Ok thanks will check but not before Monday. Feel free to investigate in the meantime if you have cycles

> Am 26.04.2019 um 14:56 schrieb Michael Nitschinger <notifications@github.com>:
> 
> Yes, when calling channel.close() my handler is triggered, when calling channel.disconnect() it is not.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

@normanmaurer no rush. won't have time to dig into it before next week either - let's sync up then.
@normanmaurer @daschl see #9098

Pull request review comments:

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 7c68b507049..1ba26fc44ca 100644
--- a/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
+++ b/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java
@@ -555,6 +555,11 @@ private void invokeConnect(SocketAddress remoteAddress, SocketAddress localAddre
 
     @Override
     public ChannelFuture disconnect(final ChannelPromise promise) {
+        if (!channel().metadata().hasDisconnect()) {
+            // Translate disconnect to close if the channel has no notion of disconnect-reconnect.
+            // So far, UDP/IP is the only transport that has such behavior.
+            return close(promise);
+        }
         if (isNotValidPromise(promise, false)) {
             // cancelled
             return promise;
@@ -563,22 +568,12 @@ public ChannelFuture disconnect(final ChannelPromise promise) {
         final AbstractChannelHandlerContext next = findContextOutbound(MASK_DISCONNECT);
         EventExecutor executor = next.executor();
         if (executor.inEventLoop()) {
-            // Translate disconnect to close if the channel has no notion of disconnect-reconnect.
-            // So far, UDP/IP is the only transport that has such behavior.
-            if (!channel().metadata().hasDisconnect()) {
-                next.invokeClose(promise);
-            } else {
-                next.invokeDisconnect(promise);
-            }
+            next.invokeDisconnect(promise);
         } else {
             safeExecute(executor, new Runnable() {
                 @Override
                 public void run() {
-                    if (!channel().metadata().hasDisconnect()) {
-                        next.invokeClose(promise);
-                    } else {
-                        next.invokeDisconnect(promise);
-                    }
+                    next.invokeDisconnect(promise);
                 }
             }, promise, null);
         }


Test patch:
diff --git a/transport/src/test/java/io/netty/channel/embedded/EmbeddedChannelTest.java b/transport/src/test/java/io/netty/channel/embedded/EmbeddedChannelTest.java
index a5dae45afa4..7c715ef13a5 100644
--- a/transport/src/test/java/io/netty/channel/embedded/EmbeddedChannelTest.java
+++ b/transport/src/test/java/io/netty/channel/embedded/EmbeddedChannelTest.java
@@ -281,6 +281,17 @@ public void testHasNoDisconnect() {
         assertNull(handler.pollEvent());
     }
 
+    @Test
+    public void testHasNoDisconnectSkipDisconnect() throws InterruptedException {
+        EmbeddedChannel channel = new EmbeddedChannel(false, new ChannelOutboundHandlerAdapter() {
+            @Override
+            public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exception {
+                promise.tryFailure(new Throwable());
+            }
+        });
+        assertFalse(channel.disconnect().isSuccess());
+    }
+
     @Test
     public void testFinishAndReleaseAll() {
         ByteBuf in = Unpooled.buffer();