This discussion is archived
3 Replies Latest reply: Jul 3, 2011 2:28 PM by 802316 RSS

Thread-Safety

872959 Newbie
Currently Being Moderated
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 Guru Moderator
    Currently Being Moderated
    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 Guru
    Currently Being Moderated
    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 Pro
    Currently Being Moderated
    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);

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points