Are Volatile and Synchronized keywords correctly used in this code?

Discussion in 'Spigot Plugin Development' started by Dimon6, Jun 29, 2015.

  1. This methods modify the HashMap "playersMap", which is accessed Asynchronously in a Runnable inner class.

    Am I using volatile and synchronized blocks correctly? Im not sure, Im trying to learn more about concurrency.
    What is the difference between using "this" or the HashMap "playersMap" as the lock for the Synchronized blcks in "onPlayerLogin" and "onPlayerLogUut" methods?

    Code (Text):
       
    private volatile HashMap<String, Player> playersMap = new HashMap<>();
    private EntityDamageByEntityEvent damageEvent = null;

    @EventHandler
        public void onPlayerLogin(PlayerJoinEvent event){
            synchronized(this){
                Player player = event.getPlayer();
                playersMap.put(player.getUniqueId().toString(), player);
            }
        }
       
        @EventHandler
        public void onPlayerLogOut(PlayerQuitEvent event){
            synchronized(this){
                Player player = event.getPlayer();
                playersMap.remove(player.getUniqueId().toString());
            }
        }
    The inncer class is like so:
    Code (Text):
    private class Checker  implements Runnable, Listener {
           
           
            @Override
            public void run() {
                Set<String> k = playersMap.keySet();
                Iterator<String> it = k.iterator();
               
                while(it.hasNext()){
                    String nextKey = it.next();
                    Player player = playersMap.get(nextKey);
                   
                   
                    ItemStack itemStack = player.getInventory().getHelmet();
                   
                    if(itemStack != null){
                        String material = itemStack.getType().toString();
                       
                        if(material.equals("PUMPKIN")){
                           
                            List<Entity> entities = player.getNearbyEntities(10, 10, 10);
                           
                            for(Entity ent : entities){
                                if(ent instanceof Monster){
                                    if(ent.isValid()){
                                       
                                       
                                        if(damageEvent != null){
                                            Entity entityDamager = damageEvent.getDamager();
                                            Entity damageReceiver = damageEvent.getEntity();
                                       
                                            if(entityDamager instanceof Monster
                                                    && damageReceiver instanceof Player){
                                               
                                                damageEvent.setCancelled(true);
                                            }

                                            System.out.println(player.getName()+" was prevented from being attacked.");
                                        }
                                       
                                        //Location playerLoc = player.getLocation();
                                        Location entLoc = ent.getLocation();
                                       
                                        Vector entVel = ent.getLocation().toVector();
                                        Vector entDirection = entLoc.getDirection();
                                        Vector newVec = entVel.multiply(entDirection).normalize();
                                       
                                        newVec.setY(0);
                                       
                                        ent.setVelocity(newVec);
                                        System.out.println("X: "+newVec.getX());
                                        System.out.println("Y: "+newVec.getY());
                                        System.out.println("Z "+newVec.getZ());
                                        System.out.println("===================");
                                    }
                                }
                            }
                        }
                    }
                }
            }
           
        }
    @EventHandler

    Code (Text):

       public void onMobAttack(EntityDamageByEntityEvent event){
         damageEvent = event;
       }
     
    Please try not using very technical words. I have not a computer engineering grade :p
     
  2. Volatile in your example is useless. Volatile only applies if you make changes to the field. I.e you change which object the field refers to. It does not apply when mutating the object itself. What volatile does is that it makes all changes to the field immediately visible to all other threads, thus establishing a happens-before relationship. Here's a good read if you're interested: http://jeremymanson.blogspot.no/2008/11/what-volatile-means-in-java.html?m=1

    The use of synchronized seems proper. Although in your example you could just as well have marked the methods themselves synchronized, as you're locking on 'this' for the entire method. The difference between locking on the map and locking on 'this' is quite simple. Each object has its own intristic lock, or monitor if you will. So the map and 'this' has two different locks. So in your example exclusively, choosing one over the other makes no difference. Just make sure to always lock on the same object, or you would have a problem.

    What is worth mentioning is that you always should consider your alternatives. In this case using a ConcurrentHashMap would be a better approach, surely? Eliminating the need for synchronisation altogether.
     
    #2 Rocoty, Jun 29, 2015
    Last edited: Jun 29, 2015
  3. Next time you post something, post your code via paste bin, or paste.md-5.net
     
  4. Code tags is fine. Preferred by most, actually.
     
    • Agree Agree x 2
  5. Thank you @Rocoty .
    The entire concurrency is a mess, there is like a whole world apart from normal Java without multithreading.
    Yes, I'll do, but why exactly?
    I indeed prefer "code" tags over pastebin :)
     
    • Like Like x 1