GameData.acquireReadLock usage?

Trevan

I'm trying to understand when GameData.acquireReadLock should be used. The javadoc for GameData says:

If you are reading the game data, you should read while you have the read lock as below.
...
The exception is delegates within a start(), end() or any method called from an IRemotePlayer through the delegates remote interface. The delegate will have a read lock for the duration of those methods.

(see https://github.com/triplea-game/triplea/blob/master/game-core/src/main/java/games/strategy/engine/data/GameData.java#L41)

I see places where it looks like GameData.acquireReadLock should be used but I don't see it being used. Take for example StrategicBombingRaidBattle#findCost. It calls GameData#getResourceList (see https://github.com/triplea-game/triplea/blob/master/game-core/src/main/java/games/strategy/triplea/delegate/battle/StrategicBombingRaidBattle.java#L992). There is no GameData.acquireReadLock nearby. And it doesn't look like this method is called within a delegate start or end, or an IRemotePlayer method. Should there be a GameData.acquireReadLock there?

And I don't see any usage of GameData.acquireReadLock inside of the AI code. I'm assuming that is because all of the code is called through an IRemotePlayer method, but I just want to make sure. Do I need to call acquireReadLock in my ai code?

And, what about changing the GameData resources to be AutoCloseable? Then, the code could be changed to "try-with-resources" instead of manually calling acquireReadLock and releaseReadLock

LaFayette

It's hard to know where the read locks are really needed vs it was mandated in that comment to always use them and then like any nice hammer, everything started to look like a nail.

Second, I've generally have thought the read locks are not guaranteeing any kind of data accuracy, perhaps just avoiding concurrent modification exception. For example:

final var someData;
try(ReadLock lock = acquireLock()) {
   someData = gameData.getSomeData();
}
// TOCTOU problem here
evaluateSomeComputation(someData);

So in short, often it seems the read lock is released too early before the data that was read under lock is consumed.

Third, it's also really questionable as a good chunk of data is not going to change, yet there is a read lock.

Bottom line: the usage of read locks is special in every case, cryptic, and really hard to tell if actually needed or not. My hope is that part of the game-save-as-deltas project, that we can start with an immutable "map-core", that is a set of data that comes from the map and truly never changes. We would instead access that data from the map, not game data, and that would reduce the amounts of read locks.

Overall, the locking is something that would be excellent to reduce and try to eliminate. Playing in bots, the locks are a frequent cause of game pauses and even crashes (for example, a player joins game, causes a game lock to be taken, the player leaves before they finish downloading the game data - the lock is then not properly released).

I think an analysis tool that I ran at one point also warned that not all locks are always released. So also at minimum, we should likely try to ensure all locks are put in a try-with-resources block to be sure they are closed, or maybe better consider moving to an API like:

var value  = gameData.executeWithReadLock(() -> supplierCodeHere)

gameData.executeWithReadLock() -> runnableCodeHere);

Trevan

So, would you be ok with removing some locks if it can be shown that the data is actually read only?

I'm currently looking at ResourceList which is what GameData#getResourceList returns. The class has only one setter and it is only called during the game parse process and a test method. The data that ResourceList stores is a map of Strings and Resources. And Resource is a value object.

ResourceList doesn't appear to need any sort of read lock since I don't see any way for it to change. If I see locks around GameData#getResourceList, can I just remove them? Would you like me to tag Resource and ResourceList somehow (such as @Value) to indicate that they are read-only objects?