Repository: netty/netty
Issue: AbstractNioBossPool may initialize more than once?
Labels: defect

The following code snippets are in 3.9.4.final.
In org.jboss.netty.channel.socket.nio.AbstractNioBossPool, method 

```
protected void init() {
    if (initialized) {
        throw new IllegalStateException("initialized already");
    }
    initialized = true;

    ...
}
```

where the definition of variable "initialized" is 

```
private volatile boolean initialized;
```

Is there concurrency hazard that may cause the init() method to be called more than once? 

If 2 threads were call the init() method simultaneously. Thread1 checks that if(initialized) is false and hang, Thread2 now checks if(initialized) is false and do the initialization. Then the Thread1 is back to run, and do the initialization again.

I think it's much better to make the variable "initialized" AtomicBoolean, because you can use atomic CAS to do the initialization exactly once:

```
private final AtomicBoolean initialized = new AtomicBoolean(false);

protected void init() {
    if (!initialized.compareAndSet(false, true)) {
        throw new IllegalStateException("initialized already");
    }

    ....
}
```


Issue comments:
@MichaelScofield sounds right... could you come up with PR ?

I've created a PR to branch 3.9: 

https://github.com/netty/netty/pull/3253

Fixed by #3253


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioBossPool.java b/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioBossPool.java
index 90bdc2c591e..08179061be2 100644
--- a/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioBossPool.java
+++ b/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioBossPool.java
@@ -22,6 +22,7 @@
 
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
 public abstract class AbstractNioBossPool<E extends Boss>
@@ -37,7 +38,7 @@ public abstract class AbstractNioBossPool<E extends Boss>
     private final Boss[] bosses;
     private final AtomicInteger bossIndex = new AtomicInteger();
     private final Executor bossExecutor;
-    private volatile boolean initialized;
+    private final AtomicBoolean initialized = new AtomicBoolean(false);
 
     /**
      * Create a new instance
@@ -66,10 +67,9 @@ public abstract class AbstractNioBossPool<E extends Boss>
     }
 
     protected void init() {
-        if (initialized) {
+        if (!initialized.compareAndSet(false, true)) {
             throw new IllegalStateException("initialized already");
         }
-        initialized = true;
 
         for (int i = 0; i < bosses.length; i++) {
             bosses[i] = newBoss(bossExecutor);
diff --git a/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioWorkerPool.java b/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioWorkerPool.java
index 6b6f0e1fa5b..ad8787c95ce 100644
--- a/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioWorkerPool.java
+++ b/src/main/java/org/jboss/netty/channel/socket/nio/AbstractNioWorkerPool.java
@@ -24,6 +24,7 @@
 
 import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -43,7 +44,7 @@ public abstract class AbstractNioWorkerPool<E extends AbstractNioWorker>
     private final AbstractNioWorker[] workers;
     private final AtomicInteger workerIndex = new AtomicInteger();
     private final Executor workerExecutor;
-    private volatile boolean initialized;
+    private final AtomicBoolean initialized = new AtomicBoolean(false);
 
     /**
      * Create a new instance
@@ -71,12 +72,10 @@ public abstract class AbstractNioWorkerPool<E extends AbstractNioWorker>
     }
 
     protected void init() {
-        if (initialized) {
+        if (!initialized.compareAndSet(false, true)) {
             throw new IllegalStateException("initialized already");
         }
 
-        initialized = true;
-
         for (int i = 0; i < workers.length; i++) {
             workers[i] = newWorker(workerExecutor);
         }


Test patch: