Repository: netty/netty
Issue: Remove AbstractChannel#doPreClose()
Labels: defect
cleanup

tl;dr this is broken and should be removed instead of fixed

doPreClose is currently called in closeFuture.setClosed():

1) During registration failure closeForcily() calls doClose before setting the closeFuture -- so doPreClose() is called after doClose().

2) During early registration failure (unable to schedule run loop) closeFuture is never set so doPreClose() is never called.

3) Exceptions in doPreClose are logged but the caller's promise is not notified.

4) doPreClose is only implemented in the local channels and that logic can be moved to close.

5) close() uses the closeFuture to determine if the socket is closed (via closeFuture.trySuccess) which calls out to user code listening to the closeFuture BEFORE closing the socket!

We should do the following:

1) Move doPreClose to doClose for local sockets.
2) Remove doPreClose and have setClosed() only call trySuccces
3) don't use closeFuture to detect if the socket is closed (use !isOpen) and call closeFuture.setClosed only after calling doClose (which now we can do because we don't have to worry about the doPreClose ordering).


Issue comments:
@jpinner what you mean with "local sockets" ?

@normanmaurer i mean LocalChannel.java LocalServerChannel.java

I'm happy to take a stab at this tonight and try to get you a pull req if you want.

Sure thing! Thanks a lot :)

Am 26.07.2013 um 07:45 schrieb Jeff Pinner notifications@github.com:

> I'm happy to take a stab at this tonight and try to get you a pull req if you want.
> 
> —
> Reply to this email directly or view it on GitHub.


Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/AbstractChannel.java b/transport/src/main/java/io/netty/channel/AbstractChannel.java
index 4f544bc67ac..638207b5b3a 100644
--- a/transport/src/main/java/io/netty/channel/AbstractChannel.java
+++ b/transport/src/main/java/io/netty/channel/AbstractChannel.java
@@ -415,6 +415,7 @@ public void run() {
                             "Force-closing a channel whose registration task was unaccepted by an event loop: {}",
                             AbstractChannel.this, t);
                     closeForcibly();
+                    closeFuture.setClosed();
                     promise.setFailure(t);
                 }
             }
@@ -440,12 +441,12 @@ private void register0(ChannelPromise promise) {
             } catch (Throwable t) {
                 // Close the channel directly to avoid FD leak.
                 closeForcibly();
+                closeFuture.setClosed();
                 if (!promise.tryFailure(t)) {
                     logger.warn(
                             "Tried to fail the registration promise, but it is complete already. " +
                                     "Swallowing the cause of the registration failure:", t);
                 }
-                closeFuture.setClosed();
             }
         }
 
@@ -514,38 +515,39 @@ public void run() {
                 return;
             }
 
-            boolean wasActive = isActive();
-            if (closeFuture.setClosed()) {
-                ChannelOutboundBuffer outboundBuffer = this.outboundBuffer;
-                this.outboundBuffer = null; // Disallow adding any messages and flushes to outboundBuffer.
+            if (closeFuture.isSuccess()) {
+                // Closed already.
+                promise.setSuccess();
+            }
 
-                try {
-                    doClose();
-                    promise.setSuccess();
-                } catch (Throwable t) {
-                    promise.setFailure(t);
-                }
+            boolean wasActive = isActive();
+            ChannelOutboundBuffer outboundBuffer = this.outboundBuffer;
+            this.outboundBuffer = null; // Disallow adding any messages and flushes to outboundBuffer.
 
-                // Fail all the queued messages
-                try {
-                    outboundBuffer.failFlushed(CLOSED_CHANNEL_EXCEPTION);
-                    outboundBuffer.close(CLOSED_CHANNEL_EXCEPTION);
-                } finally {
+            try {
+                doClose();
+                closeFuture.setClosed();
+                promise.setSuccess();
+            } catch (Throwable t) {
+                promise.setFailure(t);
+            }
 
-                    if (wasActive && !isActive()) {
-                        invokeLater(new Runnable() {
-                            @Override
-                            public void run() {
-                                pipeline.fireChannelInactive();
-                            }
-                        });
-                    }
+            // Fail all the queued messages
+            try {
+                outboundBuffer.failFlushed(CLOSED_CHANNEL_EXCEPTION);
+                outboundBuffer.close(CLOSED_CHANNEL_EXCEPTION);
+            } finally {
 
-                    deregister(voidPromise());
+                if (wasActive && !isActive()) {
+                    invokeLater(new Runnable() {
+                        @Override
+                        public void run() {
+                            pipeline.fireChannelInactive();
+                        }
+                    });
                 }
-            } else {
-                // Closed already.
-                promise.setSuccess();
+
+                deregister(voidPromise());
             }
         }
 
@@ -750,14 +752,6 @@ protected Runnable doRegister() throws Exception {
      */
     protected abstract void doDisconnect() throws Exception;
 
-    /**
-     * Will be called before the actual close operation will be performed. Sub-classes may override this as the default
-     * is to do nothing.
-     */
-    protected void doPreClose() throws Exception {
-        // NOOP by default
-    }
-
     /**
      * Close the {@link Channel}
      */
@@ -834,11 +828,6 @@ public boolean tryFailure(Throwable cause) {
         }
 
         boolean setClosed() {
-            try {
-                doPreClose();
-            } catch (Exception e) {
-                logger.warn("doPreClose() raised an exception.", e);
-            }
             return super.trySuccess();
         }
     }
diff --git a/transport/src/main/java/io/netty/channel/local/LocalChannel.java b/transport/src/main/java/io/netty/channel/local/LocalChannel.java
index cc4acb20500..13b3c57abb1 100644
--- a/transport/src/main/java/io/netty/channel/local/LocalChannel.java
+++ b/transport/src/main/java/io/netty/channel/local/LocalChannel.java
@@ -199,13 +199,12 @@ protected void doDisconnect() throws Exception {
     }
 
     @Override
-    protected void doPreClose() throws Exception {
+    protected void doClose() throws Exception {
         if (state > 2) {
             // Closed already
             return;
         }
 
-        // Update all internal state before the closeFuture is notified.
         if (localAddress != null) {
             if (parent() == null) {
                 LocalChannelRegistry.unregister(localAddress);
@@ -213,10 +212,7 @@ protected void doPreClose() throws Exception {
             localAddress = null;
         }
         state = 3;
-    }
 
-    @Override
-    protected void doClose() throws Exception {
         LocalChannel peer = this.peer;
         if (peer != null && peer.isActive()) {
             peer.unsafe().close(unsafe().voidPromise());
diff --git a/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java b/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java
index 7f279cf4409..fc6c2d031ed 100644
--- a/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java
+++ b/transport/src/main/java/io/netty/channel/local/LocalServerChannel.java
@@ -94,23 +94,17 @@ protected void doBind(SocketAddress localAddress) throws Exception {
     }
 
     @Override
-    protected void doPreClose() throws Exception {
+    protected void doClose() throws Exception {
         if (state > 1) {
             // Closed already.
             return;
         }
 
-        // Update all internal state before the closeFuture is notified.
         LocalChannelRegistry.unregister(localAddress);
         localAddress = null;
         state = 2;
     }
 
-    @Override
-    protected void doClose() throws Exception {
-        // All internal state was updated already at doPreClose().
-    }
-
     @Override
     protected Runnable doDeregister() throws Exception {
         ((SingleThreadEventExecutor) eventLoop()).removeShutdownHook(shutdownHook);


Test patch: