2
\$\begingroup\$

I have been given a task to create a thread-safe RobotCluster class. The idea is that there's a set of robots operating on multiple threads and they each submit the coordinates of a player each time they see the player in the game. I need to implement the following methods

  1. void enterCoordinate(String e, Pair<Float,Float> coordinate) - An update as seen by robot e
  2. Pair<Float,Float> getCoordinate(String e) - What is the latest coordinates at which robot e has seen the player
  3. Pair<Float,Float> waitForNextCoordinate(String e) throws InterruptedException - A method that blocks until robot e sees an update for a player that we have not read. Note that this returns early if we never read an update from robot e before.

Note that there may be multiple instances of robot e operating across many threads.

I am not very familiar with Java multi-threading and so I'd like to receive feedback on the following code. I'm particularly interested in race-conditions/bugs that I have, but I am also interested in features I am not using that'd make the code more readable/maintainable and/or use modern Java.

public final class RobotCluster {

    private final ConcurrentHashMap<String, Lock> robotLocks;
    private final ConcurrentHashMap<String, Pair<Float, Float>> coordinates;
    private final ConcurrentHashMap<String, Boolean> readLast;
    private final ConcurrentHashMap<String, Condition> robotConditions;


    public RobotCluster(){
        coordinates = new ConcurrentHashMap<>();
        readLast = new ConcurrentHashMap<>();
        robotLocks = new ConcurrentHashMap<>();
        robotConditions = new ConcurrentHashMap<>();
    }

    public void enterCoordinate(String e, Pair<Float,Float> coordinate) {
        getLockFor(e).lock();
        try{
            coordinates.put(e,coordinate);
            readLast.put(e, false);
            getConditionFor(e).signal();

        }finally {
            getLockFor(e).unlock();
        }
    }

    public Pair<Float,Float> getCoordinate(String e) {
        getLockFor(e).lock();
        try{
            readLast.put(e, true);
            return coordinates.get(e);
        }finally {
            getLockFor(e).unlock();
        }
    }


    public boolean coordinatesChanged(String e) {
        Boolean read = readLast.get(e);
        if(read == null) throw new IllegalArgumentException("Robot does not exist");
        return !read;
    }

    public Pair<Float,Float> waitForNextCoordinate(String e) throws InterruptedException{
        getLockFor(e).lock();
        try{
            while(!coordinatesChanged(e)){
                getConditionFor(e).await();
            }
            readLast.put(e, true);
            return coordinates.get(e);
        }finally {
            getLockFor(e).unlock();
        }
    }


    private Lock getLockFor(String e) {
        return robotLocks.computeIfAbsent(e, k -> new ReentrantLock());
    }

    private Condition getConditionFor(String e) {
        return robotConditions.computeIfAbsent(e, k -> getLockFor(e).newCondition());
    }
}

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Why coordinatesChanged throws if robot does not exist, but getCoordinate does not? \$\endgroup\$
    – vnp
    Commented Sep 27, 2023 at 23:49
  • \$\begingroup\$ I cannot return null as the return type of coordinatesChanged is a boolean. I considered returning false to be misleading. \$\endgroup\$
    – Bula
    Commented Sep 28, 2023 at 6:49

2 Answers 2

2
\$\begingroup\$

There are two maps used coordinates and readLast that is a clever solution to split the concurrency when read/write different information, though every method reading/writing information from/to one of the maps is also reading/writing information from/to the other map so the benefit of reduced concurrency per coordinates access is useless. coordinates and readLast could be merged in one Map<String, Pair<Pair<Float, Float>, Boolean>> and replace the value once the coordinates are read or in a map with keys of type String and values of a dedicated mutable type that encapsulates two fields one of type Pair<Float, Float> and one of type Boolean and mutate the Boolean field once coordinates are read. With a dedicated type the observer design pattern could be implemented alternatively to using Lock to register write observers if the coordinates read are stale that are updated once new coordinates are written and withdraw once they read new coordinates.

\$\endgroup\$
1
\$\begingroup\$

Finding the right data type for a problem is half the solution. And you've already found a good one.

ConcurrentHashMap implements total concurrency for reads and high concurrency for writes:

  • Read operations are guaranteed not to be blocked or block a key
  • Write operations are blocked and block other writes at the map entry level

What does it mean? When using a ConcurrentHashMap we can read the same entry from however many threads at the same time without blocking any of them. We will always get the last value that was written there. E.g. if a certain value is currently 0 and we start a process that takes 5 seconds and changes the value to 1, we will be able to read the value without waiting those 5 seconds - we will be getting 0 until the process finishes and the value is updated. Write operations block each other at entry level. We can still write into 2 entries at the same time, but if we try to write into an entry that is already being written to, we will have to wait until that write is finished.

With that said, you don't need to lock and unlock threads, as ConcurrentHashMap implementation is already protecting you from errors related to concurrent access.

Conditions that you use to implement waitForNextCoordinate can also be dumped in favor of simple wait() and notify() methods on values of readLast map.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.