Repository: netty/netty
Issue: Race condition in the NonStickyEventExecutorGroup class
Labels: defect

### Expected behavior
All `Runnable` tasks submitted through `NonStickyEventExecutorGroup` should get executed.

### Actual behavior
Occasionally the last task submitted is not getting executed.

### Steps to reproduce
The following unit-test has a non-zero chance of failure (about 1/2 of the time fail on my laptop).

```java
    @Test
    public void testRaceCondition() throws InterruptedException {
        EventExecutorGroup group = new UnorderedThreadPoolEventExecutor(1);
        NonStickyEventExecutorGroup nonStickyGroup = new NonStickyEventExecutorGroup(group, maxTaskExecutePerRun);

        try {
            EventExecutor executor = nonStickyGroup.next();

            for (int j = 0; j < 5000; j++) {
                final CountDownLatch firstCompleted = new CountDownLatch(1);
                final CountDownLatch latch = new CountDownLatch(2);
                for (int i = 0; i < 2; i++) {
                    executor.execute(new Runnable() {
                        @Override
                        public void run() {
                            firstCompleted.countDown();
                            latch.countDown();
                        }
                    });
                    Assert.assertTrue(firstCompleted.await(1, TimeUnit.SECONDS));
                }

                Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
            }

        } finally {
            nonStickyGroup.shutdownGracefully();
        }
    }
```

### Bug Description
In the `NonStickyOrderedEventExecutor` class inside the `NonStickyEventExecutorGroup` class, when the `execute(Runnable)` method (Line [314](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/concurrent/NonStickyEventExecutorGroup.java#L314)) is called, it enqueues the runnable, followed by checking the `state` to see if it needs to submit itself to the underlying `executor`. However, if at the same time the `run` thread already broke out of the polling loop, but before resetting the `state` to `NONE` Line [260](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/concurrent/NonStickyEventExecutorGroup.java#L260), then the runnable that just get enqueued will not be executed, due to the `compareAndSet` check return `false`,  until the next time `execute(Runnable)` is called again with a new runnable.

This bug manifest itself as occasionally a http client is not receiving any response from the server after request was sent if the server side is using `NonStickyEventExecutorGroup` as the event executor group for the child handler.

### Netty version
4.1.30.Final-SNAPSHOT

### JVM version (e.g. `java -version`)
1.8.0_151

### OS version (e.g. `uname -a`)
Darwin <host> 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64 x86_64


Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/concurrent/NonStickyEventExecutorGroup.java b/common/src/main/java/io/netty/util/concurrent/NonStickyEventExecutorGroup.java
index ed3a5873eca..bcc4b829964 100644
--- a/common/src/main/java/io/netty/util/concurrent/NonStickyEventExecutorGroup.java
+++ b/common/src/main/java/io/netty/util/concurrent/NonStickyEventExecutorGroup.java
@@ -259,7 +259,24 @@ public void run() {
                         }
                     } else {
                         state.set(NONE);
-                        return; // done
+                        // After setting the state to NONE, look at the tasks queue one more time.
+                        // If it is empty, then we can return from this method.
+                        // Otherwise, it means the producer thread has called execute(Runnable)
+                        // and enqueued a task in between the tasks.poll() above and the state.set(NONE) here.
+                        // There are two possible scenarios when this happen
+                        //
+                        // 1. The producer thread sees state == NONE, hence the compareAndSet(NONE, SUBMITTED)
+                        //    is successfully setting the state to SUBMITTED. This mean the producer
+                        //    will call / has called executor.execute(this). In this case, we can just return.
+                        // 2. The producer thread don't see the state change, hence the compareAndSet(NONE, SUBMITTED)
+                        //    returns false. In this case, the producer thread won't call executor.execute.
+                        //    In this case, we need to change the state to RUNNING and keeps running.
+                        //
+                        // The above cases can be distinguished by performing a
+                        // compareAndSet(NONE, RUNNING). If it returns "false", it is case 1; otherwise it is case 2.
+                        if (tasks.peek() == null || !state.compareAndSet(NONE, RUNNING)) {
+                            return; // done
+                        }
                     }
                 }
             }


Test patch:
diff --git a/common/src/test/java/io/netty/util/concurrent/NonStickyEventExecutorGroupTest.java b/common/src/test/java/io/netty/util/concurrent/NonStickyEventExecutorGroupTest.java
index 16035505e8a..b32df7eb4f7 100644
--- a/common/src/test/java/io/netty/util/concurrent/NonStickyEventExecutorGroupTest.java
+++ b/common/src/test/java/io/netty/util/concurrent/NonStickyEventExecutorGroupTest.java
@@ -25,6 +25,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
@@ -93,6 +94,35 @@ public void run() {
         }
     }
 
+    @Test
+    public void testRaceCondition() throws InterruptedException {
+        EventExecutorGroup group = new UnorderedThreadPoolEventExecutor(1);
+        NonStickyEventExecutorGroup nonStickyGroup = new NonStickyEventExecutorGroup(group, maxTaskExecutePerRun);
+
+        try {
+            EventExecutor executor = nonStickyGroup.next();
+
+            for (int j = 0; j < 5000; j++) {
+                final CountDownLatch firstCompleted = new CountDownLatch(1);
+                final CountDownLatch latch = new CountDownLatch(2);
+                for (int i = 0; i < 2; i++) {
+                    executor.execute(new Runnable() {
+                        @Override
+                        public void run() {
+                            firstCompleted.countDown();
+                            latch.countDown();
+                        }
+                    });
+                    Assert.assertTrue(firstCompleted.await(1, TimeUnit.SECONDS));
+                }
+
+                Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
+            }
+        } finally {
+            nonStickyGroup.shutdownGracefully();
+        }
+    }
+
     private static void execute(EventExecutorGroup group, CountDownLatch startLatch) throws Throwable {
         EventExecutor executor = group.next();
         Assert.assertTrue(executor instanceof OrderedEventExecutor);