#4116 Simultaneous phases, Bots, and thread safety
BotClient's gamelistener overrides gameTurnChange to spawn a new thread to compute the Bot's turn when the turn changes to the bot's turn.
This causes issues with simultaneous turns. You can see this best in a Bot vs Bot situation. We've got Bot1 and Bot2. The phase starts, and both bots start their turns, because it's simulatenous. Bot1 finishes its turn first, sends a done packet, the server replies and says it's a new turn, BotClient kicks off a new thread for Bot1, and it also kicks off a new thread for Bot2, since as long as there are valid turns left for a player, it's their turn in simultaneous play. Now Bot2 has two threads processing turns, the first one that hasn't finished plus the new turn thread. This causes huge issues, as many things, mostly notably Game, are not thread safe. In particular, this is almost guaranteed to lead to infinite loops in the physical phase.
Related
Fixed in [r10855].
The primary fix was to the BotClient's GameListener's gameTurnChange implementation. Before, it was only checking to see if Client.isMyTurn() was true, and if so, it would spawn a new turn processing thread. I added an additonal check to make sure that the player for the event was the local player. This pretty much solves the issue, however I also made BotClient.calculateMyTurn synchronized. Since it's called from multiple threads, it should be synchronized, although it shouldn't actually get called concurrently.
Just making BotClient.calculateMyTurn() synchronized by itself is not sufficient to fix the problem. While this will make subsequent threads block while the first thread processes, what will happen is, the first calculateMyTurn thread will finish, and send the turn to the server. Then, as soon as the first thread exits, the second thread starts, before the server has time to update the client with the state of the turn. This would lead to "invalid attack packets" as the second thread would try to move the same unit the first thread just moved.
This fix should remove all "invalid attack packets" when using bots in simultaneous phases. It should also eliminate NPEs and infinite loops in the physical phase processing.
Related
Commit: [r10855]
On further though, I realized this doesn't fix the problem.
First, it makes it so the bot won't move in a simultaneous fashion: it will wait for another player to move so that it receives a turn.
Secondly, the problem is still present, however a player would need to take 2+ turns for the bots 1 turn to trigger it.
This will get fixed once the PR here is merged: https://github.com/MegaMek/megamek/pull/762