diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/ChangeAttachmentChange.java b/game-app/game-core/src/main/java/games/strategy/engine/data/ChangeAttachmentChange.java index 7cb27cea5a9..1754af73530 100644 --- a/game-app/game-core/src/main/java/games/strategy/engine/data/ChangeAttachmentChange.java +++ b/game-app/game-core/src/main/java/games/strategy/engine/data/ChangeAttachmentChange.java @@ -25,7 +25,7 @@ public ChangeAttachmentChange( attachment.getAttachedTo(), attachment.getName(), newValue, - attachment.getPropertyOrThrow(property).getValue(), + DefaultAttachment.copyPropertyValue(attachment.getPropertyOrThrow(property).getValue()), property, false); } @@ -43,7 +43,7 @@ public ChangeAttachmentChange( attachment.getAttachedTo(), attachment.getName(), newValue, - attachment.getPropertyOrThrow(property).getValue(), + DefaultAttachment.copyPropertyValue(attachment.getPropertyOrThrow(property).getValue()), property, clearFirst); } diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java b/game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java index 57825c143c9..9f4e1e66234 100644 --- a/game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java +++ b/game-app/game-core/src/main/java/games/strategy/engine/data/DefaultAttachment.java @@ -6,9 +6,9 @@ import com.google.common.collect.Iterables; import games.strategy.engine.data.gameparser.GameParseException; import games.strategy.triplea.Constants; -import java.math.BigDecimal; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -79,16 +79,6 @@ protected static int getInt(final String value) { } } - /** Throws an error if format is invalid. */ - protected static BigDecimal getBigDecimal(final String value) { - try { - return new BigDecimal(value); - } catch (final NumberFormatException e) { - throw new IllegalArgumentException( - "Attachments: " + value + " is not a valid decimal value", e); - } - } - /** Throws an error if format is invalid. Must be either true or false ignoring case. */ protected static boolean getBool(final String value) { if (value.equalsIgnoreCase(Constants.PROPERTY_TRUE)) { @@ -216,4 +206,18 @@ protected static IntegerMap getIntegerMapProperty(@Nullable IntegerMap } return IntegerMap.unmodifiableViewOf(value); } + + @SuppressWarnings({"unchecked", "rawtypes"}) + public static Object copyPropertyValue(Object value) { + if (value instanceof List) { + return new ArrayList((List) value); + } else if (value instanceof IntegerMap) { + return new IntegerMap((IntegerMap) value); + } else if (value instanceof Set) { + return new HashSet((Set) value); + } else if (value instanceof Map) { + return new HashMap((Map) value); + } + return value; + } } diff --git a/game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java b/game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java index f554b4e7504..c724629e44f 100644 --- a/game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java +++ b/game-app/game-core/src/main/java/games/strategy/engine/data/GameData.java @@ -526,7 +526,7 @@ public void performChange(final Change change) { try (Unlocker ignored = acquireWriteLock()) { change.perform(this); } - dataChangeListeners.forEach(dataChangelistener -> dataChangelistener.gameDataChanged(change)); + dataChangeListeners.forEach(listener -> listener.gameDataChanged(change)); GameDataEvent.lookupEvent(change).ifPresent(this::fireGameDataEvent); } diff --git a/game-app/game-core/src/main/java/games/strategy/engine/framework/HistorySynchronizer.java b/game-app/game-core/src/main/java/games/strategy/engine/framework/HistorySynchronizer.java index d4f2433c340..e33625f17ec 100644 --- a/game-app/game-core/src/main/java/games/strategy/engine/framework/HistorySynchronizer.java +++ b/game-app/game-core/src/main/java/games/strategy/engine/framework/HistorySynchronizer.java @@ -4,7 +4,7 @@ import games.strategy.engine.data.GameData; import games.strategy.engine.data.GamePlayer; import games.strategy.engine.history.EventChild; -import java.util.ConcurrentModificationException; +import games.strategy.engine.history.HistoryWriter; import javax.swing.SwingUtilities; /** @@ -14,10 +14,8 @@ */ public class HistorySynchronizer { // Note the GameData here and the game are not the same we are keeping gameData in sync with the - // history of the game - // by listening for changes we do this because our data can change depending where in the history - // we are we want to be - // able to do this without changing the data for the game + // history of the game by listening for changes. We do this because our data can change depending + // where we are in history. We want to be able to do this without changing the data for the game. private final GameData gameData; private int currentRound; private final IGame game; @@ -27,16 +25,8 @@ public class HistorySynchronizer { public void gameDataChanged(final Change change) { SwingUtilities.invokeLater( () -> { - try { - final Change localizedChange = (Change) translateIntoMyData(change); - gameData.getHistory().getHistoryWriter().addChange(localizedChange); - } catch (ConcurrentModificationException e) { - // Temporary instrumentation to diagnose root cause of: - // https://github.com/triplea-game/triplea/issues/10274 - // Include the Change subclass name in the error. - throw new ConcurrentModificationException( - "Translating " + change.getClass().getName(), e); - } + final Change localizedChange = (Change) translateIntoMyData(change); + gameData.getHistory().getHistoryWriter().addChange(localizedChange); }); } @@ -88,14 +78,12 @@ public void stepChanged( } SwingUtilities.invokeLater( () -> { + final HistoryWriter historyWriter = gameData.getHistory().getHistoryWriter(); if (currentRound != round) { currentRound = round; - gameData.getHistory().getHistoryWriter().startNextRound(currentRound); + historyWriter.startNextRound(currentRound); } - gameData - .getHistory() - .getHistoryWriter() - .startNextStep(stepName, delegateName, player, displayName); + historyWriter.startNextStep(stepName, delegateName, player, displayName); }); } @@ -112,11 +100,8 @@ public HistorySynchronizer(final GameData data, final IGame game) { } gameData = data; gameData.forceChangesOnlyInSwingEventThread(); - data.acquireReadLock(); - try { + try (GameData.Unlocker ignored = data.acquireReadLock()) { currentRound = data.getSequence().getRound(); - } finally { - data.releaseReadLock(); } this.game = game; this.game diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/TerritoryEffectAttachment.java b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/TerritoryEffectAttachment.java index ed150fc1724..b2d629e2e73 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/attachments/TerritoryEffectAttachment.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/attachments/TerritoryEffectAttachment.java @@ -125,7 +125,13 @@ private void setMovementCostModifier(final String value) throws GameParseExcepti "movementCostModifier must have a count and at least one unitType" + thisErrorMsg()); } final Iterator iter = List.of(s).iterator(); - final BigDecimal effect = getBigDecimal(iter.next()); + final BigDecimal effect; + try { + effect = new BigDecimal(iter.next()); + } catch (final NumberFormatException e) { + throw new IllegalArgumentException( + "Attachments: " + s[0] + " is not a valid decimal value", e); + } while (iter.hasNext()) { if (movementCostModifier == null) { movementCostModifier = new HashMap<>(); diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ui/TerritoryDetailPanel.java b/game-app/game-core/src/main/java/games/strategy/triplea/ui/TerritoryDetailPanel.java index f7d0453bbed..ebbeaabc949 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/ui/TerritoryDetailPanel.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/ui/TerritoryDetailPanel.java @@ -175,18 +175,14 @@ private void territoryChanged(final @Nullable Territory territory) { final String unitsLabel; // Get the unit information under lock as otherwise they may change on the game thread causing a - // ConcurrentModificationException. Use local variable to ensure we lock/unlock the same object. - final GameData gameData = this.gameData; - gameData.acquireReadLock(); - try { + // ConcurrentModificationException. + try (GameData.Unlocker ignored = gameData.acquireReadLock()) { unitsList = UnitSeparator.getSortedUnitCategories(territory, uiContext.getMapData()); unitsLabel = "Units: " + territory.getUnits().stream() .filter(u -> uiContext.getMapData().shouldDrawUnit(u.getType().getName())) .count(); - } finally { - gameData.releaseReadLock(); } unitInfo.setText(unitsLabel); diff --git a/game-app/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java b/game-app/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java index 0d8d2f2b692..825dc6ec839 100644 --- a/game-app/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java +++ b/game-app/game-core/src/main/java/games/strategy/triplea/ui/TripleAFrame.java @@ -182,7 +182,7 @@ public final class TripleAFrame extends JFrame implements QuitHandler { private final StatPanel statsPanel; private final EconomyPanel economyPanel; private final Runnable clientLeftGame; - private ObjectivePanel objectivePanel; + private @Nullable ObjectivePanel objectivePanel; @Getter private final TerritoryDetailPanel territoryDetails; private final JPanel historyComponent = new JPanel(); private HistoryPanel historyPanel; @@ -1759,8 +1759,7 @@ private void showHistory() { inGame.set(false); setWidgetActivation(); final GameData clonedGameData; - data.acquireReadLock(); - try { + try (GameData.Unlocker ignored = data.acquireReadLock()) { // we want to use a clone of the data, so we can make changes to it as we walk up and down the // history final var cloneOptions = GameDataManager.Options.builder().withHistory(true).build(); @@ -1774,8 +1773,6 @@ private void showHistory() { } historySyncher = new HistorySynchronizer(clonedGameData, game); clonedGameData.addDataChangeListener(dataChangeListener); - } finally { - data.releaseReadLock(); } statsPanel.setGameData(clonedGameData); economyPanel.setGameData(clonedGameData);