I'm trying to implement a word game in Java, where each player takes turns extracting a number of random letters from a set, then trying to create a valid word with those letters. This is what I have so far (simplified for clarity's sake):
In the Game class, I start the game by running a thread for each player (and one for the timekeeper). I want the first player in the activePlayers list (which initially is the same as the players list) to make the first move, so I initialized the turn and turnIndex attributes to correspond to this player:
public void play()
{
this.turn = activePlayers.get(0); //the player who joined first goes first
this.turnIndex = 0; //the player's index in the ArrayList
for(Player player : players) {
new Thread(player).start();
}
new Thread(new Timekeeper()).start(); //keeps track of the game's duration
}
In the Player class, I want the players on stand-by to not do anything, and simply wait for the current player to finish their business, hence the first while loop. Then, when a player's turn has ended, I want that thread to yield the monitor to another player's thread and wait its next turn. This is how I decided to approach it:
private synchronized boolean submitWord() throws InterruptedException
{
while(game.turn != this)
{
System.out.println(this.name " is waiting their turn...");
wait();
}
Thread.sleep(1000);
List<Tile> extracted = game.getBag().extractTiles(wordLength);
if(extracted.isEmpty())
return false; //if there are no more letters to extract, the thread ends its execution
//game logic goes here - creating and validating the word
//after this player is done, the next player makes their move
game.turnIndex ;
if(game.turnIndex >= game.activePlayers.size())
game.turnIndex = 0;
game.turn = game.activePlayers.get(game.turnIndex);
notifyAll();
return true;
}
@Override
public void run()
{
do {
try {
this.running = this.submitWord();
} catch(InterruptedException e) {
System.out.println("Something went wrong with " this.name "...");
e.printStackTrace();
}
} while(this.running);
game.activePlayers.remove(this); //the player is now inactive
if(game.winner == this)
System.out.println("Winner: " this.name " [" this.score " points]");
}
However, when I try to run the program, I get something like this:
Player 2 is waiting their turn...
Player 3 is waiting their turn...
1 seconds elapsed...
Player 1: AERIAL [36 points]
Player 1 is waiting their turn...
2 seconds elapsed...
3 seconds elapsed...
4 seconds elapsed...
5 seconds elapsed...
6 seconds elapsed...
Basically, the game doesn't move past Player 1's first try, and I get stuck in an infinite loop where nothing happens. Am I not using the wait() and notifyAll() methods properly? How should I make the player threads communicate with each other?
CodePudding user response：
Just a thought, but it could be that the sleep is happening within a synchronized method: Thread.sleep is blocking other thread also, working on other method, along with itself callled inside synchronized method
CodePudding user response：
If I have understood your code correctly, that submitWord method belongs to the Player class. The keyword synchronized should be used to obtain the monitor of a shared resource to limit different threads from accessing the same resource at the same time and avoid race conditions.
In your case, you're synchronizing over a Player thread which is not the right design. You should synchronize instead over the shared resource which is the game object in this scenario. Besides, try to use synchronized blocks rather than entire synchronized methods, as the latter are more likely to block.
Within the Player's run method you should check whether the thread can acquire the game resource first with a synchronized block, if they do, then you can check whether it's the Player's turn by confronting the turn index of the game object with the Player's index. If it's not the Player's turn it invokes the wait() method; otherwise it carries on with its task by invoking submitWord.
Here, I've tweaked your code. You forgot a notify (or notifyAll) call when you were returning false in your submitWord method. That might have caused some stuck scenarios when there were no combinations available.
//Now, this method can be called only under the condition the the game's monitor lock has been already acquired. So, it can only be invoked within a synchronized block.
private boolean submitWord() {
List<Tile> extracted = game.getBag().extractTiles(wordLength);
if(extracted.isEmpty()){
//notify is more efficient than notifyAll as it causes less overhead by awakening only one random thread instead of all the ones waiting
this.game.notify();
//you were returning without notifying here... This might have caused some stucking scenarios...
return false;
}
//game logic goes here - creating and validating the word
//Rotating the turn
game.turnIndex = (this.game.turnIndex 1) % this.game.activePlayers.size();
game.turn = game.activePlayers.get(game.turnIndex);
this.game.notify();
return true;
}
@Override
public void run() {
do {
synchronized(this.game){
if (this.game.indexTurn == this.index){
this.running = this.submitWord();
//It's more efficient to check here whether the player must be removed or not as you already own the game's lock
if (!this.running){
this.game.activePlayers.remove(this);
}
} else {
try {
wait();
} catch(InterruptedException e) {
System.out.println("Something went wrong with " this.name "...");
e.printStackTrace();
}
}
}
} while(this.running);
//you should re-acquire the game's lock here since you're modifying the set of players
//synchronized(this.game){
// this.game.activePlayers.remove(this);
//}
if(this.game.winner == this){
System.out.println("Winner: " this.name " [" this.score " points]");
}
}