12542 - Mitigate race condition when selecting player sides

vassal-app/src/main/java/VASSAL/build/module/PlayerRoster.java

@@ -261,14 +261,12 @@ protected void launch() {
       return;
     }
 
-    String newSide;
-    newSide = promptForSide();
+    String newSide = promptForSide();
     if ((newSide == null) || newSide.equals(mySide)) {
       return;
     }
 
     final GameModule gm = GameModule.getGameModule();
-
     // Avoid bug that allowed gaining access to a hidden/locked side
     if (GameModule.getGameModule().getGameState().isLoadingInBackground()) {
       return;
@@ -469,19 +467,23 @@ public void finish() {
       GameModule.getGameModule().warn(Resources.getString("PlayerRoster.failed_pref_write", e.getLocalizedMessage()));
     }
 
-    final String newSide = untranslateSide(sideConfig.getValueString());
+   // Drop into standard routine, starting with checking side is still available (race condition mitigation)
+    // returns untranslated side
+    final String newSide = promptForSide(sideConfig.getValueString());
+
     if (newSide != null) {
       if (GameModule.getGameModule().isMultiplayerConnected()) {
-        final Command c = new Chatter.DisplayText(GameModule.getGameModule().getChatter(), Resources.getString(GlobalOptions.getInstance().chatterHTMLSupport() ? "PlayerRoster.joined_side_2" : "PlayerRoster.joined_side", GameModule.getGameModule().getPrefs().getValue(GameModule.REAL_NAME), newSide));
+        final Command c = new Chatter.DisplayText(GameModule.getGameModule().getChatter(), Resources.getString(GlobalOptions.getInstance().chatterHTMLSupport() ? "PlayerRoster.joined_side_2" : "PlayerRoster.joined_side", GameModule.getGameModule().getPrefs().getValue(GameModule.REAL_NAME), translateSide(newSide)));
         c.execute();
       }
 
       final Add a = new Add(this, GameModule.getActiveUserId(), GlobalOptions.getInstance().getPlayerId(), newSide);
       a.execute();
       GameModule.getGameModule().getServer().sendToOthers(a);
+
+      getLaunchButton().setVisible(getMySide() != null);
+      pickedSide = true;
     }
-    getLaunchButton().setVisible(getMySide() != null);
-    pickedSide = true;
   }
 
   @Override
@@ -757,52 +759,87 @@ public List<String> getAvailableSides() {
   }
 
   protected String promptForSide() {
+    return promptForSide("");
+  }
+
+  protected String promptForSide(String checkSide) {
+
     final ArrayList<String> availableSides = new ArrayList<>(sides);
     final ArrayList<String> alreadyTaken = new ArrayList<>();
+    boolean fromWizard;
+    String newSide = "";
 
-    for (final PlayerInfo p : players) {
-      alreadyTaken.add(p.side);
+    if (checkSide != null && checkSide.isEmpty()) {
+      fromWizard = false;
+    }
+    else {
+      fromWizard = true;
+      newSide = checkSide;
     }
 
-    availableSides.removeAll(alreadyTaken);
+    while (newSide != null) { // Loops until a valid side is found or op is canceled (repeats side check to minimuse race condition window)
+
+      for (final PlayerInfo p : players) {
+        alreadyTaken.add(p.side);
+      }
 
-    // If a "real" player side is available, we want to offer "the next one" as the default, rather than observer.
-    // Thus hotseat players can easily cycle through the player positions as they will appear successively as the default.
-    // Common names for Solitaire players (Solitaire, Solo, Referee) do not count as "real" player sides, and will be skipped.
-    // If we have no "next" side available to offer, we stay with the observer side as our default offering.
-    boolean found = false;       // If we find a usable side
-    final String mySide = getMySide(); // Get our own side, so we can find the "next" one
-    final int myidx = (mySide != null) ? sides.indexOf(mySide) : -1; // See if we have a current non-observe side.
-    int i = (myidx >= 0) ? ((myidx + 1) % sides.size()) : 0;   // If we do, start looking in the "next" slot, otherwise start at beginning.
-    for (int tries = 0; i != myidx && tries < sides.size(); i = (i + 1) % sides.size(), tries++) { // Wrap-around search of sides
-      final String s = sides.get(i);
-      if (!alreadyTaken.contains(s) && !isSoloSide(s)) {
-        found = true; // Found an available slot that's not our current one and not a "solo" slot.
+      /*
+       The while loop ensures that the selected side is re-checked here and only returned if the side is still available.
+       This prevents players switching to the same side if they enter the switch-side dialogue (below) at the same time,
+       narrowing the race condition window to network latency.
+      */
+      if (!newSide.isEmpty() && !alreadyTaken.contains(newSide)) {
+        // side is returned in English for sharing in the game.
+        newSide = untranslateSide(newSide);
         break;
       }
-    }
+      else {
+        // Set up for another try...
+        availableSides.clear();
+        availableSides.addAll(sides);
+      }
 
-    final String nextChoice = found ? sides.get(i) : translatedObserver; // This will be our defaulted choice for the dropdown.
+      availableSides.removeAll(alreadyTaken);
+      String nextChoice = translatedObserver; // This will be our defaulted choice for the dropdown.
+
+      // If a "real" player side is available, we want to offer "the next one" as the default, rather than observer.
+      // Thus hotseat players can easily cycle through the player positions as they will appear successively as the default.
+      // Common names for Solitaire players (Solitaire, Solo, Referee) do not count as "real" player sides, and will be skipped.
+      // If we have no "next" side available to offer, we stay with the observer side as our default offering.
+      if (!fromWizard) {
+        final String mySide = getMySide(); // Get our own side, so we can find the "next" one
+        final int myidx = (mySide != null) ? sides.indexOf(mySide) : -1; // See if we have a current non-observe side.
+        int i = (myidx >= 0) ? ((myidx + 1) % sides.size()) : 0;   // If we do, start looking in the "next" slot, otherwise start at beginning.
+        for (int tries = 0; i != myidx && tries < sides.size(); i = (i + 1) % sides.size(), tries++) { // Wrap-around search of sides
+          final String s = sides.get(i);
+          if (!alreadyTaken.contains(s) && !isSoloSide(s)) {
+            nextChoice = sides.get(i); // Found an available slot that's not our current one and not a "solo" slot.
+            break;
+          }
+        }
+      }
 
-    availableSides.add(0, translatedObserver);
+      availableSides.add(0, translatedObserver);
 
-    final GameModule g = GameModule.getGameModule();
-    String newSide = (String) JOptionPane.showInputDialog(
-      g.getPlayerWindow(),
-      Resources.getString("PlayerRoster.switch_sides", getMyLocalizedSide()), //$NON-NLS-1$
-      Resources.getString("PlayerRoster.choose_side"), //$NON-NLS-1$
-      JOptionPane.QUESTION_MESSAGE,
-      null,
-      availableSides.toArray(new String[0]),
-      nextChoice // Offer calculated most likely "next side" as the default
-    );
+      final GameModule g = GameModule.getGameModule();
 
-    // sides must always be stored internally in English.
-    if (translatedObserver.equals(newSide)) {
-      newSide = OBSERVER;
-    }
-    else {
-      newSide = untranslateSide(newSide);
+      newSide = (String) JOptionPane.showInputDialog(
+              g.getPlayerWindow(),
+              newSide.isEmpty() ? Resources.getString("PlayerRoster.switch_sides", getMyLocalizedSide()) : Resources.getString("PlayerRoster.switch_sides2", newSide, getMyLocalizedSide()), //$NON-NLS-1$
+              Resources.getString("PlayerRoster.choose_side"), //$NON-NLS-1$
+              JOptionPane.QUESTION_MESSAGE,
+              null,
+              availableSides.toArray(new String[0]),
+              nextChoice // Offer calculated most likely "next side" as the default
+      );
+
+      if (translatedObserver.equals(newSide)) { // Observer returns here, other returns are checked once more.
+        newSide = OBSERVER;
+        break;
+      }
+      else {
+        alreadyTaken.clear(); // prepare to loop again for exit check
+      }
     }
     return newSide;
   }

vassal-app/src/main/resources/VASSAL/i18n/VASSAL.properties

@@ -1025,6 +1025,7 @@ PlayerRoster.join_another_side=Join another side
 PlayerRoster.give_up_position=Give up your position as %1$s?
 PlayerRoster.join_game_as=Join game as which side?  
 PlayerRoster.switch_sides=Your current side is %1$s. Switch to which side?
+PlayerRoster.switch_sides2=%1$s is now unavailable. Your current side is %2$s. Switch to which side?
 PlayerRoster.choose_side=Choose side
 PlayerRoster.observer=<observer>
 PlayerRoster.solitaire=Solitaire

vassal-app/src/main/resources/VASSAL/i18n/VASSAL_da.properties

@@ -1023,6 +1023,7 @@ PlayerRoster.join_another_side=Skift til en anden side
 PlayerRoster.give_up_position=Vil du opgive din position som %1$s?
 PlayerRoster.join_game_as=Join spillet p� hvilken side?
 PlayerRoster.switch_sides=Din nuv�rende side er %1$s. Skift til hvilken side?
+PlayerRoster.switch_sides2=%1$s er nu ikke tilg�ngelig. Din nuv�rende side er %2$s. Skift til hvilken side?
 PlayerRoster.choose_side=V�lg side
 PlayerRoster.observer=<tilskuer>
 PlayerRoster.solitaire=Solitaire

vassal-app/src/main/resources/VASSAL/i18n/VASSAL_de.properties

@@ -547,6 +547,8 @@ PlayerRoster.become_observer=Werde zum Beobachter
 PlayerRoster.join_another_side=Wechsle zu anderer Seite
 PlayerRoster.give_up_position=Nicht mehr weiterspielen als %1$s?
 PlayerRoster.join_game_as=Spiele auf welcher Seite?
+PlayerRoster.switch_sides=Ihre aktuelle Seite ist %1$s. Auf welche Seite wechseln?
+PlayerRoster.switch_sides2=%1$s ist nicht verfügbar. Ihre aktuelle Seite ist %2$s. Auf welche Seite wechseln?
 PlayerRoster.choose_side=Wähle Seite
 PlayerRoster.observer=<Beobachter>
 

vassal-app/src/main/resources/VASSAL/i18n/VASSAL_es.properties

@@ -704,6 +704,7 @@ PlayerRoster.join_another_side=Unirse a otro bando
 PlayerRoster.give_up_position=¿Dejar su lugar como %1$s?
 PlayerRoster.join_game_as=Unirse a la partida como... 
 PlayerRoster.switch_sides=Tu bando actual es %1$s. ¿Cambiar de bando?
+PlayerRoster.switch_sides2=%1$s no está disponible. Tu bando actual es %2$s. ¿Cambiar de bando?
 PlayerRoster.choose_side=Seleccione su bando
 PlayerRoster.observer=<Observador>
 PlayerRoster.solitaire=Solitario

vassal-app/src/main/resources/VASSAL/i18n/VASSAL_fr.properties

@@ -782,6 +782,7 @@ PlayerRoster.join_another_side=Changer de camp
 PlayerRoster.give_up_position=Abandonner sa position comme %1$s
 PlayerRoster.join_game_as=Rejoindre la partie en prenant quel camp ?
 PlayerRoster.switch_sides=Votre camp actuel est %1$s. Basculer vers quel camp?
+PlayerRoster.switch_sides2=%1$s n'est pas disponible. Votre camp actuel est %2$s. Basculer vers quel camp?
 PlayerRoster.choose_side=Choisissez votre camp
 PlayerRoster.observer=<Observateur>
 PlayerRoster.solitaire=Solitaire

vassal-app/src/main/resources/VASSAL/i18n/VASSAL_it.properties

@@ -601,6 +601,8 @@ PlayerRoster.become_observer=Diventa osservatore
 PlayerRoster.join_another_side=Cambia fazione
 PlayerRoster.give_up_position=Abbandoni la fazione di %1$s?
 PlayerRoster.join_game_as=Scelta fazione  
+PlayerRoster.switch_sides=Il tuo lato attuale è %1$s. Passare da che parte?
+PlayerRoster.switch_sides2=%1$s non è disponibile. Il tuo lato attuale è %2$s. Passare da che parte?
 PlayerRoster.choose_side=Scegli la fazione
 PlayerRoster.observer=<osservatore>
 PlayerRoster.referee=Arbitro

vassal-app/src/main/resources/VASSAL/i18n/VASSAL_nl.properties

@@ -527,6 +527,8 @@ PlayerRoster.become_observer=Wordt kijker
 PlayerRoster.join_another_side=Kies een andere kant
 PlayerRoster.give_up_position=Geef uw positie op als %1$s?
 PlayerRoster.join_game_as=Doe mee aan welke kant?  
+PlayerRoster.switch_sides=Je huidige kant is %1$s. Naar welke kant overstappen?
+PlayerRoster.switch_sides2=%1$s is niet beschikbaar. Je huidige kant is %2$s. Naar welke kant overstappen?
 PlayerRoster.choose_side=Kies kant
 PlayerRoster.observer=<kijker>