Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbstractNioBossPool may initialize more than once? #3249

Closed
MichaelScofield opened this issue Dec 15, 2014 · 3 comments
Closed

AbstractNioBossPool may initialize more than once? #3249

MichaelScofield opened this issue Dec 15, 2014 · 3 comments
Assignees
Labels
Milestone

Comments

@MichaelScofield
Copy link

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");
    }

    ....
}
@normanmaurer
Copy link
Member

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

@MichaelScofield
Copy link
Author

I've created a PR to branch 3.9:

#3253

normanmaurer pushed a commit that referenced this issue Dec 16, 2014
…tractNioBoss(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 #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.
@normanmaurer
Copy link
Member

Fixed by #3253

@normanmaurer normanmaurer self-assigned this Dec 16, 2014
@normanmaurer normanmaurer added this to the 3.9.6.Final milestone Dec 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants