xXDylanDog13Xx / kryonet

Automatically exported from code.google.com/p/kryonet
1 stars 1 forks source link

Incorrect locking code in Server.java #8

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
in Server.update, the code block looks like this:

synchronized (updateLock) { // Blocks to avoid a select while the selector is 
used to bind the server connection.
        }

if (timeout > 0) {
selector.select(timeout);
} else {
selector.selectNow();
}

That is an empty synchronized block.
It seems what you wanted was :

synchronized (updateLock) { // Blocks to avoid a select while the selector is 
used to bind the server connection.
    if (timeout > 0) {
    selector.select(timeout);
    } else {
    selector.selectNow();
    }
}

Original issue reported on code.google.com by prashant...@gmail.com on 31 May 2011 at 3:41

GoogleCodeExporter commented 9 years ago
The updateLock is a bit tricky, and you are right to be wary when you see an 
empty synchronized block. It is used to prevent a select from occurring while 
the server socket is being bound (NIO has many idiosyncrasies!). Note that 
everywhere updateLock is synchronized, the code first calls selector.wakeup(). 
This causes selector.select(timeout) to return, and another select cannot occur 
because it will stop at the empty synchronized block.

The selector is synchronized internally, updateLock doesn't protect it.

If the locking were implemented as you suggest, selector.select(timeout) would 
be holding the updateLock monitor. This means any code that wants to obtain the 
updateLock monitor will have to wait for the timeout.

Original comment by nathan.s...@gmail.com on 31 May 2011 at 6:36