Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

12542 - Mitigate race condition when selecting player sides #12667

Merged
Merged
Show file tree
Hide file tree
Changes from 67 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
91ef123
Provides facility to set the hot-seat default side via a module Globa…
riverwanderer Sep 4, 2023
fc94dfb
Provides facility to set the hot-seat default side via a module Globa…
riverwanderer Sep 4, 2023
026178c
Provides facility to set the hot-seat default side via a module Globa…
riverwanderer Sep 4, 2023
b03eef8
Provides facility to set the hot-seat default side via a module Globa…
riverwanderer Sep 4, 2023
07e3128
Validate VassalNextSide - check that it is an available side, rather …
riverwanderer Sep 4, 2023
007630c
Validate VassalNextSide - check that it is an available side, rather …
riverwanderer Sep 4, 2023
c5dcadb
Validate VassalNextSide - check that it is an available side, rather …
riverwanderer Sep 4, 2023
59ea275
Use null-safe test on VassalNextSide.
riverwanderer Sep 4, 2023
d153db4
Ref manual updated. Supercedes equivalent update in PR #12636.
riverwanderer Sep 4, 2023
afc7319
Attempt race condition check for non-Wizard switch side.
riverwanderer Sep 7, 2023
9662e1d
Attempt race condition check for non-Wizard switch side.
riverwanderer Sep 7, 2023
cdb5d2c
Attempt race condition check for non-Wizard switch side.
riverwanderer Sep 7, 2023
77a3b0e
Attempt race condition check for non-Wizard switch side.
riverwanderer Sep 7, 2023
af03551
Corrected an indent level.
riverwanderer Sep 7, 2023
f030ebf
Corrected an indent level.
riverwanderer Sep 7, 2023
5d7344b
Don't re-check observer.
riverwanderer Sep 7, 2023
048ab12
Try a message.
riverwanderer Sep 7, 2023
aa5ed65
Try a message. INcludes translations, except Japanese (placeholders a…
riverwanderer Sep 7, 2023
fa8497c
Try a message. INcludes translations, except Japanese (placeholders a…
riverwanderer Sep 7, 2023
95c1628
Try a message. INcludes translations, except Japanese (placeholders a…
riverwanderer Sep 7, 2023
09352eb
Try a message. INcludes translations, except Japanese (placeholders a…
riverwanderer Sep 7, 2023
b330796
Reset alreadyTaken list before exit check.
riverwanderer Sep 8, 2023
9a2bbd1
Correct import issue, revert japanese placeholders, improve comments.
riverwanderer Sep 8, 2023
169c43f
Need to reset availableSides as well.
riverwanderer Sep 8, 2023
d77d193
Attempt to insert side race check into Wizard finish step.
riverwanderer Sep 8, 2023
e908007
Attempt to insert side race check into Wizard finish step.
riverwanderer Sep 8, 2023
99caa0b
Attempt to insert side race check into Wizard finish step.
riverwanderer Sep 8, 2023
e51cdd4
Attempt to insert side race check into Wizard finish step.
riverwanderer Sep 8, 2023
405dd19
Remove redundant setting of availableSides in finish().
riverwanderer Sep 9, 2023
7976438
Experiment with setProblem()
riverwanderer Sep 9, 2023
5896768
Loop within Finish until an available side is selected.
riverwanderer Sep 9, 2023
90f4c27
Integrate finish() and promptForSide().
riverwanderer Sep 9, 2023
e4ab000
Integrate finish() and promptForSide().
riverwanderer Sep 9, 2023
4a7add4
Integrate finish() and promptForSide().
riverwanderer Sep 9, 2023
5e33313
Trying to get promptForSide() working with a parameter.
riverwanderer Sep 10, 2023
61e3b72
Temporarily rename promptForSide() for testing.
riverwanderer Sep 10, 2023
9d6404e
Try varargs.
riverwanderer Sep 10, 2023
4aa0175
Try copying to promptForSide2().
riverwanderer Sep 10, 2023
da81b02
promptForSide2(...) is now backend to promptForSide().
riverwanderer Sep 10, 2023
1e3a7da
promptForSide2(...) is now backend to promptForSide().
riverwanderer Sep 10, 2023
7ba9548
Try without using varArgs.
riverwanderer Sep 10, 2023
5176c0c
Try without using varArgs.
riverwanderer Sep 10, 2023
a3acfd9
Try overloading promptForSide().
riverwanderer Sep 10, 2023
5b5b167
Try overloading promptForSide().
riverwanderer Sep 10, 2023
b7c6611
Retry.
riverwanderer Sep 10, 2023
a7a7233
Retry.
riverwanderer Sep 10, 2023
be88d9c
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
riverwanderer Sep 10, 2023
b02b321
Retry.
riverwanderer Sep 10, 2023
5dd76bc
Retry.
riverwanderer Sep 10, 2023
62b2203
Fix PMDs and translate side in "joined" message.
riverwanderer Sep 10, 2023
c111b2b
Swap out invalid characters in PlayerRoster.switch_sides/2 properties.
riverwanderer Sep 10, 2023
4810c6f
Resubmit as UTF-8.
riverwanderer Sep 10, 2023
9aa05ef
Restore cc.png
riverwanderer Sep 10, 2023
26467de
Amend comment.
riverwanderer Sep 13, 2023
6a297a8
Merge branch '12544_nextSideChoice' of https://github.com/riverwander…
riverwanderer Sep 14, 2023
48950b8
Merge with PR #12646 (nextSide)
riverwanderer Sep 14, 2023
35eff8c
Resolve gameModule prior to entering loop. Disable VassalNextChoice.
riverwanderer Sep 14, 2023
c51128b
Merge branch 'master' of https://github.com/vassalengine/vassal into …
riverwanderer Sep 14, 2023
2135177
Merge branch 'master' of https://github.com/vassalengine/vassal into …
riverwanderer Sep 14, 2023
3e877da
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
riverwanderer Sep 14, 2023
aa90bb3
Merge branch 'master' of https://github.com/vassalengine/vassal into …
riverwanderer Sep 14, 2023
857d91e
Merge branch 'master' of https://github.com/vassalengine/vassal into …
riverwanderer Sep 14, 2023
1986378
Merge branch 'master' of https://github.com/vassalengine/vassal into …
riverwanderer Sep 14, 2023
c075ac6
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
riverwanderer Sep 14, 2023
7b1840b
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
riverwanderer Sep 14, 2023
079d27b
Update documentation to remove Solitaire changes which may layer on t…
riverwanderer Sep 14, 2023
5a5e01d
Merge remote-tracking branch 'origin/12542-side-switch-race-mitigatio…
riverwanderer Sep 14, 2023
0cbee71
Merge branch 'master' of https://github.com/vassalengine/vassal into …
riverwanderer Sep 19, 2023
a561316
Re-aligned to master.
riverwanderer Sep 19, 2023
9604c83
Re-aligned to master.
riverwanderer Sep 19, 2023
12fe724
Attempt block button is no sides and to prevent null playerSide in game.
riverwanderer Sep 19, 2023
e80631d
Attempt block button is no sides and to prevent null playerSide in game.
riverwanderer Sep 19, 2023
070cfd2
Attempt block button is no sides and to prevent null playerSide in game.
riverwanderer Sep 19, 2023
ba88b09
Merge branch '12542-side-switch-race-mitigation' of https://github.co…
riverwanderer Sep 19, 2023
0f47f77
Attempt block button if no sides and to prevent null playerSide in game.
riverwanderer Sep 19, 2023
a928de9
Restore always do setLaunchBtton() in PlayerRoster(). Wrong place to …
riverwanderer Sep 19, 2023
4f315af
null playerside check correction.
riverwanderer Sep 19, 2023
dafa3c2
null playerside check correction. Observer needs to be filtered from …
riverwanderer Sep 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 89 additions & 41 deletions vassal-app/src/main/java/VASSAL/build/module/PlayerRoster.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
uckelman marked this conversation as resolved.
Show resolved Hide resolved
pickedSide = true;
}
getLaunchButton().setVisible(getMySide() != null);
pickedSide = true;
}

@Override
Expand Down Expand Up @@ -757,52 +759,98 @@ 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 = "";
final GameModule g = GameModule.getGameModule();

for (final PlayerInfo p : players) {
alreadyTaken.add(p.getLocalizedSide());
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.getLocalizedSide());
}

// 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 = translateSide(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) {

// Module controlled method: set VassalNextSide (a Module Global Property)
// If not found / available method 2 is used to find likely next side
// Reserved property VassalNextSide may override hotseat default; must be an available side in english
// if (!StringUtils.isEmpty((String) g.getProperty("VassalNextSide"))) {
// nextChoice = translateSide((String) g.getProperty("VassalNextSide"));
//}
// Until VassalNextSide is implemented, nextChoice is observer by default.
// To implement, must also handle restoring observer if no other nextChoice is found.

if (!availableSides.contains(nextChoice)) {

final String mySide = translateSide(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 = s; // 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
);
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
);

// sides must always be stored internally in English.
if (translatedObserver.equals(newSide)) {
newSide = OBSERVER;
}
else {
newSide = untranslateSide(newSide);
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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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?
BrentEaston marked this conversation as resolved.
Show resolved Hide resolved
PlayerRoster.choose_side=V�lg side
PlayerRoster.observer=<tilskuer>
PlayerRoster.solitaire=Solitaire
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
Binary file modified vassal-app/src/test/resources/test-images/cc.png
riverwanderer marked this conversation as resolved.
Show resolved Hide resolved
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ Simply type a name for each side and refer to that name in the restricted compon
Only one player may be assigned to a side.
When joining a game, players will be prompted to take one of the remaining available sides.
Any number of observers (players who belong to no side) are allowed.

The <<Toolbar.adoc#Retire,Retire>> or <<Toolbar.adoc#SwitchSides,Switch Sides>> button, in the main controls toolbar, allows a player to relinquish their side (making it available to the next player joining the game). You can specify the text, icon, and mouse-over tooltip for the toolbar button.

The Switch Sides component includes hot-seat support. Vassal will determine the next side in order, excluding non-player sides "Solo", "Solitaire", "Moderator" and "Referee".

|===

==== <<GlobalOptions.adoc#top,Global Options>>
Expand Down