Correctly handle non IOException during read in NioServerSocketChannel

Motivation:

Our code was not correct in AbstractNioMessageChannel.closeOnReadError(....) which lead to the situation that we always tried to continue reading no matter what exception was thrown when using the NioServerSocketChannel. Also even on an IOException we should check if the Channel itself is still active or not and if not stop reading.

Modifications:

Fix closeOnReadError impl and added test.

Result:

Correctly stop reading on NioServerSocketChannel when error happens during read.

transport/src/main/java/io/netty/channel/nio/AbstractNioMessageChannel.java

@@ -172,11 +172,19 @@ public abstract class AbstractNioMessageChannel extends AbstractNioChannel {
     }
 
     protected boolean closeOnReadError(Throwable cause) {
-        // ServerChannel should not be closed even on IOException because it can often continue
-        // accepting incoming connections. (e.g. too many open files)
-        return cause instanceof IOException &&
-                !(cause instanceof PortUnreachableException) &&
-                !(this instanceof ServerChannel);
+        if (!isActive()) {
+            // If the channel is not active anymore for whatever reason we should not try to continue reading.
+            return true;
+        }
+        if (cause instanceof PortUnreachableException) {
+            return false;
+        }
+        if (cause instanceof IOException) {
+            // ServerChannel should not be closed even on IOException because it can often continue
+            // accepting incoming connections. (e.g. too many open files)
+            return !(this instanceof ServerChannel);
+        }
+        return true;
     }
 
     /**

transport/src/main/java/io/netty/channel/socket/nio/NioServerSocketChannel.java

@@ -200,4 +200,10 @@ public class NioServerSocketChannel extends AbstractNioMessageChannel
             clearReadPending();
         }
     }
+
+    // Override just to to be able to call directly via unit tests.
+    @Override
+    protected boolean closeOnReadError(Throwable cause) {
+        return super.closeOnReadError(cause);
+    }
 }

transport/src/test/java/io/netty/channel/socket/nio/NioServerSocketChannelTest.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2018 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.channel.socket.nio;
+
+import io.netty.channel.EventLoopGroup;
+import io.netty.channel.nio.NioEventLoopGroup;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ServerSocketChannel;
+
+public class NioServerSocketChannelTest {
+
+    @Test
+    public void testCloseOnError() throws Exception {
+        ServerSocketChannel jdkChannel = ServerSocketChannel.open();
+        NioServerSocketChannel serverSocketChannel = new NioServerSocketChannel(jdkChannel);
+        EventLoopGroup group = new NioEventLoopGroup(1);
+        try {
+            group.register(serverSocketChannel).syncUninterruptibly();
+            serverSocketChannel.bind(new InetSocketAddress(0)).syncUninterruptibly();
+            Assert.assertFalse(serverSocketChannel.closeOnReadError(new IOException()));
+            Assert.assertTrue(serverSocketChannel.closeOnReadError(new IllegalArgumentException()));
+            serverSocketChannel.close().syncUninterruptibly();
+        } finally {
+            group.shutdownGracefully();
+        }
+    }
+}