Repository: netty/netty
Issue: Code suggestion about shutdownGracefully

Here's the current [code](https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java#L557), and the following is a code snippet:

```java
boolean inEventLoop = inEventLoop();
boolean wakeup;
int oldState;
for (;;) {
    if (isShuttingDown()) {
        return terminationFuture();
    }
    int newState;
    wakeup = true;
    oldState = STATE_UPDATER.get(this);
    if (inEventLoop) {
        newState = ST_SHUTTING_DOWN;
    } else {
        switch (oldState) {
            case ST_NOT_STARTED:
            case ST_STARTED:
                newState = ST_SHUTTING_DOWN;
                break;
            default:
                newState = oldState;
                wakeup = false;
        }
    }
    if (STATE_UPDATER.compareAndSet(this, oldState, newState)) {
        break;
    }
}
gracefulShutdownQuietPeriod = unit.toNanos(quietPeriod);
gracefulShutdownTimeout = unit.toNanos(timeout);
```

Would it be better to code like this?

```java
int oldState;
for (;;) {
    oldState = STATE_UPDATER.get(this);
    if (oldState >= ST_SHUTTING_DOWN) {
        return terminationFuture();
    }

    if (STATE_UPDATER.compareAndSet(this, oldState, ST_SHUTTING_DOWN)) {
        break;
    }
}
gracefulShutdownQuietPeriod = unit.toNanos(quietPeriod);
gracefulShutdownTimeout = unit.toNanos(timeout);
```

Issue comments:
please submit a PR if you this this is an improvement. I don't see `wakeup` taken into account.
@Scottmitch Thank you  for  your advice, here is the PR #6267 

Pull request review comments:
This is a change in behavior, are you sure this is desired? Previously the state was changed to shutdown before the thread was started, now we unconditionally start the thread before attempting shutdown. Perhaps now the thread will be allowed to process jobs that it previously wouldn't have been allowed to.
@Scottmitch , Please check it again.
I think you just moved this check from line 611 ... however I wonder if the intention is that this method should be able to "force the shutdown" even if the state is `ST_SHUTTING_DOWN` (@normanmaurer - WDYT). If that is the case this method should only exit prematurely if `state >= ST_SHUTDOWN`.
(for the line above): minor optimization ... `isShuttingDown()` is called above which will use the state, and you are also getting the state below in a loop. you could get the state outside the loop, use it above, and then refresh it at the end of the loop.

```java
int oldState = STATE_UPDATER.get(this);
if (isShuttingDown(state)) {
  return terminationFuture();
}

for(;;) {
//try atomic set
  oldState = STATE_UPDATER.get(this);
}

@Override
public boolean isShuttingDown() {
  return isShuttingDown(STATE_UPDATER.get(this));
}

private static boolean isShuttingDown(int state) {
  return state >= ST_SHUTDOWN;
}
```
Yes, maybe you are right!
@Scottmitch , how about we just remove the shutdown check before the for loop?
this is a change in behavior. the new code will always try to change the state but the old code, but the old code only changed the state in the event loop. @normanmaurer - WDYT (here is the original change [1])

[1] https://github.com/netty/netty/commit/16a85e6cca46cfdcfe07c9e76a3b935c72c5ec1d#diff-cde1b8aa1fe0c3a083412f79421bafcfR501

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 e90358cffd0..22be86b9b0f 100644
--- a/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
+++ b/common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
@@ -547,37 +547,18 @@ public Future<?> shutdownGracefully(long quietPeriod, long timeout, TimeUnit uni
             throw new NullPointerException("unit");
         }
 
-        if (isShuttingDown()) {
-            return terminationFuture();
-        }
-
-        boolean inEventLoop = inEventLoop();
-        boolean wakeup;
         int oldState;
         for (;;) {
-            if (isShuttingDown()) {
-                return terminationFuture();
-            }
-            int newState;
-            wakeup = true;
             oldState = STATE_UPDATER.get(this);
-            if (inEventLoop) {
-                newState = ST_SHUTTING_DOWN;
-            } else {
-                switch (oldState) {
-                    case ST_NOT_STARTED:
-                    case ST_STARTED:
-                        newState = ST_SHUTTING_DOWN;
-                        break;
-                    default:
-                        newState = oldState;
-                        wakeup = false;
-                }
+            if (oldState >= ST_SHUTTING_DOWN) {
+              return terminationFuture();
             }
-            if (STATE_UPDATER.compareAndSet(this, oldState, newState)) {
+
+            if (STATE_UPDATER.compareAndSet(this, oldState, ST_SHUTTING_DOWN)) {
                 break;
             }
         }
+
         gracefulShutdownQuietPeriod = unit.toNanos(quietPeriod);
         gracefulShutdownTimeout = unit.toNanos(timeout);
 
@@ -585,10 +566,7 @@ public Future<?> shutdownGracefully(long quietPeriod, long timeout, TimeUnit uni
             doStartThread();
         }
 
-        if (wakeup) {
-            wakeup(inEventLoop);
-        }
-
+        wakeup(inEventLoop());
         return terminationFuture();
     }
 
@@ -604,31 +582,14 @@ public void shutdown() {
             return;
         }
 
-        boolean inEventLoop = inEventLoop();
-        boolean wakeup;
         int oldState;
         for (;;) {
-            if (isShuttingDown()) {
-                return;
-            }
-            int newState;
-            wakeup = true;
             oldState = STATE_UPDATER.get(this);
-            if (inEventLoop) {
-                newState = ST_SHUTDOWN;
-            } else {
-                switch (oldState) {
-                    case ST_NOT_STARTED:
-                    case ST_STARTED:
-                    case ST_SHUTTING_DOWN:
-                        newState = ST_SHUTDOWN;
-                        break;
-                    default:
-                        newState = oldState;
-                        wakeup = false;
-                }
+            if (oldState >= ST_SHUTTING_DOWN) {
+                return;
             }
-            if (STATE_UPDATER.compareAndSet(this, oldState, newState)) {
+
+            if (STATE_UPDATER.compareAndSet(this, oldState, ST_SHUTDOWN)) {
                 break;
             }
         }
@@ -637,9 +598,7 @@ public void shutdown() {
             doStartThread();
         }
 
-        if (wakeup) {
-            wakeup(inEventLoop);
-        }
+        wakeup(inEventLoop());
     }
 
     @Override


Test patch: