Piet Souris wrote:I use a Lock too, as Stephan did, but I did not use Conditions (never used so far).
You'd use
Condition in the same situation where in the olden days you would use
Object.wait().
Lock is a replacement for the
synchronized keyword, not for
wait().
But I'm not sure about some things: is a Lock necessary?
Yes. Each player is performing non-atomic operations on variables that are shared with other players. Without the lock, two types of problems may arise:
First, other players may observe that individual operations occur out-of-order. For instance, if player 1 receives the ball and has reached the maximum score, it will first add itself to the list of "ranked" players before it passes the ball to another player. However, the player that you passed the ball to may not observe the change to the result list, and if it also reaches the maximum score, it might "steal" the rank of player 1, or even worse, completely overwrite player 1's operation so that player 1 doesn't appear in the final ranking at all. Worse yet, its possible that player 2 removes itself from the list of players after player 1 has checked
players.isEmpty(), causing player 1's thread to crash with an
IndexOutOfBoundsException when it calls
players.get(r).
Secondly, updates may not be observed by other threads at all. When player 1 calls
getBall() on player 2, player 2 may never actually observe the change to
hasBall if you don't synchronize the two threads using
Lock.lock(). In fact, your current code may theoretically display this problem because the
hasBall check before the
sleep() call is not synchronized.
You didn't put the
hasBall check inside the lock, because it would cause your application to deadlock with all players waiting for the active thread to stop sleeping, which will never happen because none of the blocked threads will ever give it a ball. This is exactly what
Condition is for: It allows the active thread to release the lock while it waits for another thread to update the application state. However, in this particular case the
hasBall check is an atomic operation, so instead of using
Condition you can solve this issue simply by making the
hasBall field
volatile.
How about the visibility of all players and their field "hasBall"?
As I explained above, access to the
result and
players lists is protected by the
Lock, so you're good there. You haven't protected the
hasBall check, but you can fix this by making the field
volatile.
The code seems to work, but if anyone has some useful comments, I'd like to hear them.
Make your classes final and package private. None of your classes or members need to be public, with the exception of the main() and run() methods.Seriously, don't start threads from constructors. All that a constructor should do is initialize the object's fields. Move the code that starts execution of your players to a separate method that you call after the constructor returns.Always call ExecutorService.shutdown() from a finally block. Otherwise, if an unexpected exception occurs or if the main thread times out while waiting for termination, the thread pool might keep running and your application won't terminate.Your ternary expression can be simplified to just i == r.Make hasBall volatile.Use a better name than getBall(). It is extremely confusing because it looks like a simple getter, but it's actually a mutator. Use a name such as setHasBall() or receiveBall() or something.If you don't expect a particular checked exception to be thrown, enforce it with an assertion.Your assumption that "// we have the ball!" is incorrect. You used an if-statement, so the player will only wait 100 ms once and then try to acquire the lock regardless of whether they have the ball.You shouldn't use comments like that in your code anyway. If you feel the need to clarify what a piece of code does, lift it to its own method.Call Lock.lock() outside of the try-statement. Even though lock() doesn't throw any exceptions, it's a good habit because similar methods like lockInterruptibly() do. If the attempt is made inside the try-statement, you will end up trying to unlock a lock that you haven't locked, which will result in an IllegalMonitorStateException.Don't override equals() and hashCode() in classes that don't represent value types.