Repository: netty/netty
Issue: SingleThreadEventExecutor ignores startThread failures
Labels: defect

### Expected behavior
SingleThreadEventExecutor should throw an exception when it can't start the thread second time.

### Actual behavior
When `doStartThread` throws an exception, e.g. due to the actual executor being depleted of threads and throwing in its rejected execution handler, the STEE ends up in started state anyway. If we try to execute another task in this executor, it will be queued but the thread won't be started anymore and the task will linger forever.

### Netty version
4.1.9

### JVM version (e.g. `java -version`)
1.8

Issue comments:
@rvansa looking now
@rvansa PTAL  #7492
Fixed by https://github.com/netty/netty/pull/7492

Pull request review comments:
`j.u.c.RejectedExecutionHandler` is not limited to throwing REEs, why not `Throwable`?
We try to not introduce more Throwable catches if we rethrow these. Maybe `Exception` ?
Sounds good.
@rvansa PTAL again just pushed
optional:

`if (cause instanceof Error)) {`

I think I would like to keep it as it is for now just in case someone creates a special `Throwable` sub-type

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java b/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
index 510d05aea49..d1de03fe6e3 100644
--- a/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
+++ b/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
@@ -16,6 +16,7 @@
 package io.netty.util.concurrent;
 
 import io.netty.util.internal.ObjectUtil;
+import io.netty.util.internal.PlatformDependent;
 import io.netty.util.internal.SystemPropertyUtil;
 import io.netty.util.internal.UnstableApi;
 import io.netty.util.internal.logging.InternalLogger;
@@ -582,7 +583,18 @@ public Future<?> shutdownGracefully(long quietPeriod, long timeout, TimeUnit uni
         gracefulShutdownTimeout = unit.toNanos(timeout);
 
         if (oldState == ST_NOT_STARTED) {
-            doStartThread();
+            try {
+                doStartThread();
+            } catch (Throwable cause) {
+                STATE_UPDATER.set(this, ST_TERMINATED);
+                terminationFuture.tryFailure(cause);
+
+                if (!(cause instanceof Exception)) {
+                    // Also rethrow as it may be an OOME for example
+                    PlatformDependent.throwException(cause);
+                }
+                return terminationFuture;
+            }
         }
 
         if (wakeup) {
@@ -634,7 +646,18 @@ public void shutdown() {
         }
 
         if (oldState == ST_NOT_STARTED) {
-            doStartThread();
+            try {
+                doStartThread();
+            } catch (Throwable cause) {
+                STATE_UPDATER.set(this, ST_TERMINATED);
+                terminationFuture.tryFailure(cause);
+
+                if (!(cause instanceof Exception)) {
+                    // Also rethrow as it may be an OOME for example
+                    PlatformDependent.throwException(cause);
+                }
+                return;
+            }
         }
 
         if (wakeup) {
@@ -837,7 +860,12 @@ protected final void reject(Runnable task) {
     private void startThread() {
         if (state == ST_NOT_STARTED) {
             if (STATE_UPDATER.compareAndSet(this, ST_NOT_STARTED, ST_STARTED)) {
-                doStartThread();
+                try {
+                    doStartThread();
+                } catch (Throwable cause) {
+                    STATE_UPDATER.set(this, ST_NOT_STARTED);
+                    PlatformDependent.throwException(cause);
+                }
             }
         }
     }


Test patch:
diff --git a/common/src/test/java/io/netty/util/concurrent/SingleThreadEventExecutorTest.java b/common/src/test/java/io/netty/util/concurrent/SingleThreadEventExecutorTest.java
index 3be7dc8f831..55981b24060 100644
--- a/common/src/test/java/io/netty/util/concurrent/SingleThreadEventExecutorTest.java
+++ b/common/src/test/java/io/netty/util/concurrent/SingleThreadEventExecutorTest.java
@@ -21,6 +21,8 @@
 import java.util.Collections;
 import java.util.Set;
 import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -28,6 +30,48 @@
 
 public class SingleThreadEventExecutorTest {
 
+    @Test
+    public void testWrappedExecutureIsShutdown() {
+        ExecutorService executorService = Executors.newSingleThreadExecutor();
+
+        SingleThreadEventExecutor executor = new SingleThreadEventExecutor(null, executorService, false) {
+            @Override
+            protected void run() {
+                while (!confirmShutdown()) {
+                    Runnable task = takeTask();
+                    if (task != null) {
+                        task.run();
+                    }
+                }
+            }
+        };
+
+        executorService.shutdownNow();
+        executeShouldFail(executor);
+        executeShouldFail(executor);
+        try {
+            executor.shutdownGracefully().syncUninterruptibly();
+            Assert.fail();
+        } catch (RejectedExecutionException expected) {
+            // expected
+        }
+        Assert.assertTrue(executor.isShutdown());
+    }
+
+    private static void executeShouldFail(Executor executor) {
+        try {
+            executor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    // Noop.
+                }
+            });
+            Assert.fail();
+        } catch (RejectedExecutionException expected) {
+            // expected
+        }
+    }
+
     @Test
     public void testThreadProperties() {
         final AtomicReference<Thread> threadRef = new AtomicReference<Thread>();