Duplicated keys in Tree Map when accesing concurrently

Question

Can someone find the concurrency error in this code? The code is working perfectly fine for one thread but as soon I start 2 threads at the same times and I call the addScore method, it is adding duplicates to the tree Map.

The pojo with the comparedTO overrided is as follows:

public final class UserHighScore implements Comparable<UserHighScore>{

    private final int userId;
    private final int value;

    public UserHighScore(int userId, int value) {
        this.userId = userId;
        this.value = value;
    }

    public int getUserId() {
        return userId;
    }

    public int getValue() {
        return value;
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == this) return true;
        if (!(obj instanceof UserHighScore)) {
            return false;
        }
        UserHighScore userHighScore = (UserHighScore) obj;
        return userHighScore.userId==userId;
    }

    @Override
    public int compareTo(UserHighScore uh) {
        if(uh.getUserId()==this.getUserId()) return 0;
        if(uh.getValue()>this.getValue()) return 1;
        return -1;
    }
}

This is the code that I'm using to simulate a User making requests:

class User implements Runnable
{

    private ScoreServiceImpl scoreService=ScoreServiceImpl.getInstance();

    CountDownLatch latch;
    public User(CountDownLatch latch)
    {
        this.latch = latch;
    }

    @Override
    public void run() {


        for(int i=0;i<5;i++) {
            scoreService.addScore(3,Integer.parseInt(Thread.currentThread().getName()),ThreadLocalRandom.current().nextInt(50000));
        }
        System.out.println(scoreService.getHighScoreList(3));
    }
}

And the main method to create the threads is:

public static void main(String[] args) throws InterruptedException {

    SpringApplication.run(RestclientApplication.class, args);

    CountDownLatch latch = new CountDownLatch(1);
    User user1=new User(latch);
    User user2=new User(latch);
    Thread t1=new Thread(user1);
    Thread t2=new Thread(user2);
    t1.setName("1");
    t2.setName("2");
    t1.start();
    t2.start();
    //latch.countDown();
}

Answer 1

Your compareTo is screwy. You can get the same result single threaded with something like this

    ScoreServiceImpl.getInstance().addScore(0,1,4);
    ScoreServiceImpl.getInstance().addScore(0,1,12);
    ScoreServiceImpl.getInstance().addScore(0,0,10);
    ScoreServiceImpl.getInstance().addScore(0,0,3);

Tree sets work by divide and conquer, it checks the guy in the middle first. (which will will be 1, 4) and since the userIds don't match it doesn't compare them and compares the values instead. if it had compared the userids it would have gone left but instead it went right and only compared items with a userid of one

You can either always compare on both values or always compare on just userId but you can't switch back and forth.

@Override
public int compareTo(UserHighScore uh) {
    return Integer.compare(userId, uh.userId);
}