From f0503ffb020a4283ac93ec07a96f0aba69064923 Mon Sep 17 00:00:00 2001 From: luofucong Date: Tue, 16 Dec 2014 15:15:29 +0800 Subject: [PATCH] Prevent the potential concurrency hazard of the initialization in AbstractNioBoss(Worker)Pool. Motivation: During the reading of the source codes of Netty 3.9.5.Final, I thought there might have a lurking concurrency bug in the AbstractNioBossPool#init method, of which a single volatile variable cannot correctly control the initialization sequence happened exactly only once under concurrency environment. Also I found there's already a much more elegant and concurrency safe way to do the work like this, I decided to make this PR. (Please refer to the discussion in https://github.com/netty/netty/issues/3249.) Modifications: Change the type of the variable that control the initialization from "volatile boolean" to "final AtomicBoolean". Result: The potential concurrency hazard of the initialization in AbstractNioBoss(Worker)Pool will be eliminated. --- .../netty/channel/socket/nio/AbstractNioBossPool.java | 6 +++--- .../netty/channel/socket/nio/AbstractNioWorkerPool.java | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) 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 90bdc2c591..08179061be 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 org.jboss.netty.util.internal.ExecutorUtil; 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 @@ -37,7 +38,7 @@ public abstract class AbstractNioBossPool 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 } 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 6b6f0e1fa5..ad8787c95c 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 org.jboss.netty.util.internal.ExecutorUtil; 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 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 } 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); }