EsotericSoftware / kryonet

TCP/UDP client/server library for Java, based on Kryo
BSD 3-Clause "New" or "Revised" License
1.82k stars 419 forks source link

Incorrect locking code in Server.java #9

Closed ghost closed 10 years ago

ghost commented 10 years ago

From prashant...@gmail.com on May 31, 2011 05:41:44

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: http://code.google.com/p/kryonet/issues/detail?id=8

ghost commented 10 years ago

From nathan.s...@gmail.com on May 31, 2011 11:36:38

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.

Status: WontFix