3 Replies Latest reply: Jul 3, 2011 4:28 PM by 802316 RSS

    Thread-Safety

    872959
      Please can you check the following code and advise thread-safety? If it is not thread-safe, could you please advise where it is breaking?

           
           
           public class ClassA {

                private List<Player> players;


                public ClassA() {
                     this.players = Collections.synchronizedList(new ArrayList<Player>());
                }


                
                public Player play(Player player){
                int score = 0;
                     .
                     .
                     .

                     if (players.contains(player)) {
                          player = players.get(players.indexOf(player));
                          player.addScore(score);
                     } else {
                          player.addScore(score);
                          players.add(player);
                     }
                return player;

                }
                
           }
        • 1. Re: Thread-Safety
          DarrylBurke
          Moderator advice: Please read the announcement(s) at the top of the forum listings and the FAQ linked from every page. They are there for a purpose.

          Then edit your post and format the code correctly.

          db
          • 2. Re: Thread-Safety
            796440
            It is not threadsafe.

            For instance, between
            if (players.contains(player)) {
            and
            player = players.get(players.indexOf(player));
            another thread could modify the list, so that it may no longer contain the player in question.

            Not only that, but in the single line
            player = players.get(players.indexOf(player));
            between the indexOf() call and the get() call, the list could be modified, so you might get the wrong index, or an index that no longer exists.

            There may be other problems (in fact I'd be willing to bet that there are), but those are the ones that jumped out at me.

            And yeah, in the future, please do as DB suggested, and use code tags to make your code readable.
            • 3. Re: Thread-Safety
              802316
              If you want a collection you can lookup is a thread safe matter you can use a ConcurrentMap
              final ConcurrentMap<Player, Player> players = new ConcurrentHashMap<Player, Player>();
              
              players.putIfAbsent(player, player);
              player = players.get(player);