Repository: netty/netty
Issue: Make NIO and EPOLL autoReadClear consistent
Labels: improvement

[AbstractNioChannel readPending](https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java#L64) is volatile because it is directly set when [autoRead is cleared](https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/socket/nio/NioSocketChannel.java#L364). The EPOLL transport takes an alternative approach and avoids the need for `readPending` to be volatile. [EpollChannelConfig calls clearEpollIn()](https://github.com/netty/netty/blob/4.1/transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollChannelConfig.java#L175) which will run immediately if on the event loop and if not execute a runnable to run on the event loop.


Issue comments:
If the EPOLL approach is acceptable this would be preferred as it would remove a volatile which is written to frequently in the event loop. The only hesitation is that [AbstractNioChannel](https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java#L133) exposes `readPending` with protected accessor methods ... however I'm not sure if anyone should be relying on the volatile nature of this variable...

@normanmaurer @trustin - WDYT?

This was discovered during investigation in https://github.com/netty/netty/pull/5067 and removing a volatile will become more important as the call frequency to `setReadPending` will increase (if the changes in this PR are "correct").

@Scottmitch I would agree that the EPOLL approach is "ok". @trustin WDYT ?


Pull request review comments:
@Scottmitch why not just remove this and always call `setReadPendin(false)` ?

~~@normanmaurer - done~~

@normanmaurer - Of course this can be done but `clearReadPending` uses a cached Runnable `clearReadPendingRunnable` and `setReadPending` has been deprecated in favor of `clearReadPending`. If we don't care about `clearReadPendingRunnable` we can do this.

ok fine with me as well..


Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java
index 981f7c8b15f..d69358167b1 100644
--- a/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java
+++ b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java
@@ -65,7 +65,7 @@ public abstract class AbstractNioChannel extends AbstractChannel {
     private final Runnable clearReadPendingRunnable = new Runnable() {
         @Override
         public void run() {
-            readPending = false;
+            clearReadPending0();
         }
     };
 
@@ -149,17 +149,17 @@ protected void setReadPending(final boolean readPending) {
         if (isRegistered()) {
             EventLoop eventLoop = eventLoop();
             if (eventLoop.inEventLoop()) {
-                this.readPending = readPending;
+                setReadPending0(readPending);
             } else {
                 eventLoop.execute(new OneTimeTask() {
                     @Override
                     public void run() {
-                        AbstractNioChannel.this.readPending = readPending;
+                        setReadPending0(readPending);
                     }
                 });
             }
         } else {
-            this.readPending = readPending;
+            setReadPending0(readPending);
         }
     }
 
@@ -170,16 +170,28 @@ protected final void clearReadPending() {
         if (isRegistered()) {
             EventLoop eventLoop = eventLoop();
             if (eventLoop.inEventLoop()) {
-                readPending = false;
+                clearReadPending0();
             } else {
                 eventLoop.execute(clearReadPendingRunnable);
             }
         } else {
             // Best effort if we are not registered yet clear readPending. This happens during channel initialization.
-            readPending = false;
+            clearReadPending0();
         }
     }
 
+    private void setReadPending0(boolean readPending) {
+        this.readPending = readPending;
+        if (!readPending) {
+            ((AbstractNioUnsafe) unsafe()).removeReadOp();
+        }
+    }
+
+    private void clearReadPending0() {
+        readPending = false;
+        ((AbstractNioUnsafe) unsafe()).removeReadOp();
+    }
+
     /**
      * Return {@code true} if the input of this {@link Channel} is shutdown
      */


Test patch: