1 2 Previous Next 15 Replies Latest reply: Aug 1, 2009 1:34 PM by 782681 RSS

    Synchronize on objects held in non-final variables?

    843789
      [This thread|http://forums.sun.com/thread.jspa?messageID=10775877#10775877] and post got me thinking:
      linuxisbest.eric wrote:
      I have noticed in my textbook that it creates a String in two ways:
      String a = new String("Hello");
      and
      String a = "Hello";
      Which one should I use? Is there any practical difference?
      YoungWinston wrote:Pick door number two linux ... unless you plan on synchronizing on it, that is.
      Winston
      Is it best practice to only synchronize on objects held in final variables?

      Edited by: Encephalopathic on Jul 26, 2009 5:46 AM
        • 1. Re: Synchronize on objects held in non-final variables?
          baftos
          I think there are two questions here.

          1. Should the member variable be final? In 99% (100%?) of the cases yes. Otherwise the referenced object may be changed, probably by mistake, and threads that assume synchronization on the same object are actually not. My Netbeans warns me "Synchronization on non-final field".

          2. About why the peculiar
          String a = new String("Hello");
          may actually make sense (see last reply of the thread you quote)? I think you will find the explanation here:[http://forums.sun.com/thread.jspa?threadID=5355923]. See the OP, the sentence "lock(?myLock?) is a problem because any other code in the process using the same string, will share the same lock."
          • 2. Re: Synchronize on objects held in non-final variables?
            843789
            baftos wrote:
            1. Should the member variable be final? In 99% (100%?) of the cases yes. Otherwise the referenced object may be changed, probably by mistake, and threads that assume synchronization on the same object are actually not. My Netbeans warns me "Synchronization on non-final field".
            That was what I was thinking.
            2. About why the peculiar
            String a = new String("Hello");
            may actually make sense (see last reply of the thread you quote)? ....See the OP, the sentence "lock(?myLock?) is a problem because any other code in the process using the same string, will share the same lock."
            Also this makes sense, and is something I hadn't thought of.
            Thanks
            • 3. Re: Synchronize on objects held in non-final variables?
              800427
              Use the stuff in java.util.concurrent.locks if you need locks.
              Otherwise, don't use Strings for locks.
              A simple
              static final Object LOCK = new Object();
              avoids the common locking errors associated with string literals.
              • 4. Re: Synchronize on objects held in non-final variables?
                843789
                beders wrote:
                Use the stuff in java.util.concurrent.locks...
                Good points. Thanks.
                • 5. Re: Synchronize on objects held in non-final variables?
                  800308
                  beders wrote:
                  Use the stuff in java.util.concurrent.locks if you need locks.
                  Otherwise, don't use Strings for locks.
                  A simple
                  static final Object LOCK = new Object();
                  avoids the common locking errors associated with string literals.
                  That's locking on the Class... not an instance of the class.... probably not what you wanted ;-)
                  • 6. Re: Synchronize on objects held in non-final variables?
                    YoungWinston
                    Encephalopathic wrote:
                    Is it best practice to only synchronize on objects held in final variables?
                    I found a fairly succinct reply to that question [here.|http://www.coderanch.com/t/233943/Threads-Synchronization/java/Synchronization-non-final-field]
                    But the reason for my post is that Java shares string literals, so if you sync on a String like:
                    String lock = "Lock";
                    your code will lock all strings that were set up with the same literal.

                    HIH

                    Winston
                    • 7. Re: Synchronize on objects held in non-final variables?
                      800427
                      We were talking about locks using String literals and the dangers of creating a JVM wide locking problem.
                      What kind of locks do you had in mind?
                      • 8. Re: Synchronize on objects held in non-final variables?
                        3004
                        YoungWinston wrote:
                        Encephalopathic wrote:
                        Is it best practice to only synchronize on objects held in final variables?
                        I found a fairly succinct reply to that question [here.|http://www.coderanch.com/t/233943/Threads-Synchronization/java/Synchronization-non-final-field]
                        But the reason for my post is that Java shares string literals, so if you sync on a String like:
                        String lock = "Lock";
                        your code will lock all strings that were set up with the same literal.
                        What do you mean "lock all strings that were set up with the same literal"? Syncing always does exactly the same thing: It waits until the lock for the object in question is available, and then it obtains that lock. We typically say it "locks a block of code (or a method) on the object", not that it "locks the object". There is no "all strings" here, just one.

                        If you meant to say that every sync block that uses a reference to the string literal "Lock" will obtain the same single String object's lock, then, yes you are correct, though I believe that has already been stated earlier in the thread.
                        • 9. Re: Synchronize on objects held in non-final variables?
                          3004
                          corlettk wrote:
                          beders wrote:
                          Use the stuff in java.util.concurrent.locks if you need locks.
                          Otherwise, don't use Strings for locks.
                          A simple
                          static final Object LOCK = new Object();
                          avoids the common locking errors associated with string literals.
                          That's locking on the Class... not an instance of the class.... probably not what you wanted ;-)
                          Er, that's locking on an instance of the Object class. All instances of the containing class for that declaration of course share the same lock, which may or may not be what was desired (though typically it's not).
                          • 10. Re: Synchronize on objects held in non-final variables?
                            800308
                            jverd wrote:
                            corlettk wrote:
                            beders wrote:
                            Use the stuff in java.util.concurrent.locks if you need locks.
                            Otherwise, don't use Strings for locks.
                            A simple
                            static final Object LOCK = new Object();
                            avoids the common locking errors associated with string literals.
                            That's locking on the Class... not an instance of the class.... probably not what you wanted ;-)
                            Er, that's locking on an instance of the Object class. All instances of the containing class for that declaration of course share the same lock, which may or may not be what was desired (though typically it's not).
                            Ummm... Yeah, I think that's what I said... at-least that's what I was I trying (and failing) to say... thank you for clarifying my rather awkward, incomplete treatment.

                            And in the interests of getting stuff crystal clear (yeah, yeah)... the problem with final String lock = "lock"; is that if anyone anywhere-else happens to use the same interned-string (i.e. the String object having the value "lock") then you're unconsciously locking there threads, and they're locking yours... correct? So your application is likely to run like a complete dog at odd moments, and the root cause would be very hard to fathom... hence I think the recomendation is to NEVER use an interned (or otherwise cached) anything (String and byte-sized-Integers being the obvious examples) as a the-object-on-which-to-synchronize... and anyway, what was wrong with the tried-and-true-simple-solution? Ergo: final Object lock = new Object();

                            Sometimes we programmers are just too clever for our own good.

                            Cheers. Keith.
                            • 11. Re: Synchronize on objects held in non-final variables?
                              782681
                              ...
                              Is it best practice to only synchronize on objects held in final variables?
                              Above [best practices|http://en.wikipedia.org/wiki/Best_practices] apply to cases when lock is not intended to change.

                              I think that there could be cases when lock is actually intended to change and therefore shouldn't be final. Here's an example of caching code I've seen recently:
                              //...
                                private final ConcurrentMap<Integer, Object> pendings
                                    = new ConcurrentHashMap<Integer, Object>();
                              
                                public Object loadEntity(int entityId) {
                                  Object lock = new Object(); // shouldn't be final, see below
                                  Object prev = pendings.putIfAbsent(entityId, lock);
                                  if(prev != null) {
                                    // without using prev lock, there would be a chance
                                    //  for concurrent threads to perform redundant invocation
                                    //  of retrieveEntityFromSomewhereSlowly for same entityId
                                    lock = prev;
                                  }
                                  Object res;
                                  synchronized (lock) {
                                    res = getEntityFromCacheQuickly(entityId);
                                    if (res == null) {
                                      // cache miss - retrieve entity slowly:
                                      res = retrieveEntityFromSomewhereSlowly(entityId);
                                    }
                                  }
                                  pendings.remove(entityId);
                                }
                              //...
                              • 12. Re: Synchronize on objects held in non-final variables?
                                843789
                                gnat wrote:
                                I think that there could be cases when lock is actually intended to change and therefore shouldn't be final.
                                Another excellent point in a very interesting thread. Thanks all!
                                • 13. Re: Synchronize on objects held in non-final variables?
                                  800308
                                  gnat wrote:
                                  ...
                                  Is it best practice to only synchronize on objects held in final variables?
                                  Above [best practices|http://en.wikipedia.org/wiki/Best_practices] apply to cases when lock is not intended to change.

                                  I think that there could be cases when lock is actually intended to change and therefore shouldn't be final. Here's an example of caching code I've seen recently:
                                  //...
                                  private final ConcurrentMap<Integer, Object> pendings
                                  = new ConcurrentHashMap<Integer, Object>();
                                  
                                  public Object loadEntity(int entityId) {
                                  Object lock = new Object(); // shouldn't be final, see below
                                  Object prev = pendings.putIfAbsent(entityId, lock);
                                  if(prev != null) {
                                  // without using prev lock, there would be a chance
                                  //  for concurrent threads to perform redundant invocation
                                  //  of retrieveEntityFromSomewhereSlowly for same entityId
                                  lock = prev;
                                  }
                                  Object res;
                                  synchronized (lock) {
                                  res = getEntityFromCacheQuickly(entityId);
                                  if (res == null) {
                                  // cache miss - retrieve entity slowly:
                                  res = retrieveEntityFromSomewhereSlowly(entityId);
                                  }
                                  }
                                  pendings.remove(entityId);
                                  }
                                  //...
                                  gnat,

                                  Huh? Please could you try to talk me through that code... At first glance, I don't follow it... I think it's still got "unprotected race conditions", but after reading it through several times (in the course of trying to frame a post saying basically "I don't think that will work, and here's why" I now think I understand how it does work (almost all the time), but I'd just really really like some validation of my mis/understanding.

                                  So... Here's how I think that works... please could you verify or repudiate my understanding.
                                  package forums;
                                  
                                  import java.util.Random;
                                  import java.util.Map;
                                  import java.util.HashMap;
                                  import java.util.concurrent.ConcurrentMap;
                                  import java.util.concurrent.ConcurrentHashMap;
                                  
                                  class GnatsThreadsafeCache
                                  {
                                    private static final Random random = new Random();
                                  
                                    // pending locks := entityId --> lock
                                    private final ConcurrentMap<Integer, Object> pendingLocks = new ConcurrentHashMap<Integer, Object>();
                                  
                                    // cache := entityId --> entity 
                                    //          1        --> "1"
                                    private final Map<Integer, Object> cache = new HashMap<Integer, Object>();
                                  
                                    public Object loadEntity(int entityId) {
                                      Object lock = new Object(); // shouldn't be final, see below
                                      // putIfAbsent: If the specified key is not already associated with a value,
                                      //  associate it with the given value. Returns the previous value associated 
                                      //  with the specified key, or null if there was no mapping for the key.
                                      Object previousLock = pendingLocks.putIfAbsent(entityId, lock);
                                      if(previousLock != null) {
                                        // without using previousLock lock, there would be a chance
                                        //  for concurrent threads to perform redundant invocation
                                        //  of retrieveEntityFromSomewhereSlowly for same entityId
                                        lock = previousLock;
                                      }
                                      Object entity;
                                      synchronized (lock) {
                                        entity = getEntityFromCacheQuickly(entityId);
                                        if (entity == null) {
                                          // cache miss - retrieve entity slowly:
                                          entity = retrieveEntityFromSomewhereSlowly(entityId);
                                          addEntityToCache(entityId, entity);
                                        }
                                      }
                                      pendingLocks.remove(entityId);
                                      return entity;
                                    }
                                  
                                    private Object getEntityFromCacheQuickly(int entityId) {
                                      DEBUG("> getEntityFromCacheQuickly("+entityId+")");
                                      Object entity = cache.get(entityId);
                                      DEBUG("< getEntityFromCacheQuickly returns "+entity);
                                      return entity;
                                    }
                                  
                                    private Object retrieveEntityFromSomewhereSlowly(int entityId) {
                                      DEBUG("> retrieveEntityFromSomewhereSlowly("+entityId+")");
                                      try{Thread.sleep(random.nextInt(1000));}catch(InterruptedException e){System.err.println(e);}
                                      Object entity = Integer.toString(entityId);
                                      DEBUG("< retrieveEntityFromSomewhereSlowly returns "+entity);
                                      return entity;
                                    }
                                  
                                    private void addEntityToCache(int entityId, Object entity) {
                                      DEBUG("| addEntityToCache("+entityId+", "+entity+")");
                                      cache.put(entityId, entity);
                                    }
                                  
                                    public static void main(String[] args)
                                    {
                                      final GnatsThreadsafeCache cache = new GnatsThreadsafeCache();
                                      for (int i=0; i<10; i++) {
                                        new Thread(
                                          new Runnable() {
                                            @Override
                                            public void run() {
                                              int i = random.nextInt(4);
                                              Object entity = cache.loadEntity(i);
                                              DEBUG("~ "+i+"-->"+entity);
                                            }
                                          }
                                        , "Thread"+i).start();
                                      }
                                    }
                                  
                                    private static void DEBUG(String message) {
                                      System.err.println(Thread.currentThread().getName()+": "+message);
                                    }
                                  }
                                  How it works:
                                  1. create a new lock object.
                                  2. check if entityId exists in the previousLock list... that's a ConcurrentMap so the putIfAbsent operation is atomic (i.e. syncronised on the previousLock object).
                                      ~  This operation is "garanteed fast"; ergo it involves no heavy "business object" creation/retrieval.
                                  3. Now, if the entityId already exists in the previousLock list we syncronize on it, allowing one-thread-at-a-time to traverse "the crytical block" where we check if the-entity associated with the-given-entityId is allready cached, and retrieve it, otherwise create a new one (which will take an arbitrary "long time").

                                  So what? Well I think it means that we've reduced contention to the individual entity level, as apposed to the "collection level"; as you would get if you just used a "standard" ConcurrentMap as the cache... which means that we've (probably) increased the throughput because each thread is blocked only upon those other threads which definately would stuff-them-up (waiting for the heavy "business object" creation routine to complete); as apposed to those that just might stuff them up (but won't because they're actually retrieving different entities).

                                  In short: It's a way of blocking on distinct keys?

                                  Is that correct?

                                  Thanx in advance for your thoughts.

                                  Cheers. Keith.

                                  Edited by: corlettk on 1/08/2009 11:05 ~~ Removed extranious cache.get from retrieveEntityFromSomewhereSlowly.
                                  • 14. Re: Synchronize on objects held in non-final variables?
                                    782681
                                    Thanks for an excellent SSCCE - it was a pleasure to work with. I think you got it right - the point is blocking on distinct keys.

                                    Driven by your note, I also tried to imagine an alternative - I mean locking on the "collection level", as opposed to the the individual entity level.

                                    Here's what I've got:
                                    import java.util.*;
                                    import java.util.concurrent.*;
                                    
                                    class CorlettsThreadsafeCache
                                    {
                                      private static final Random random = new Random();
                                    
                                      // cache := entityId --> entity
                                      //          1        --> 1,1,1,...,1
                                      private final ConcurrentMap<Integer, Object> cache
                                          = new ConcurrentHashMap<Integer, Object>();
                                    
                                      private final Object lock = new Object(); // lock is final but... see below
                                    
                                      public Object loadEntity(int entityId) {
                                        Object entity = null;
                                        synchronized (lock) { // final lock => ugly block, see below
                                          entity = cache.get(entityId);
                                          if (entity == null) {
                                            // cache miss - retrieve entity slowly:
                                            entity = retrieveEntityFromSomewhereSlowly(entityId);
                                            addEntityToCache(entityId, entity);
                                            // ugly block:
                                            //   note while we're doing all this, no other thread can
                                            //   load from cache, even if "its" entity is already there
                                          } else {
                                            DEBUG("> getEntityFromCacheQuickly("+entityId+")");
                                            DEBUG("< getEntityFromCacheQuickly returns "+entity);
                                          }
                                        }
                                        return entity;
                                      }
                                    
                                      // interface to simulate large entity objects
                                      public interface LargeDataStorage { int getElement(int index); }
                                    
                                      private Object retrieveEntityFromSomewhereSlowly(final int entityId) {
                                        DEBUG("> retrieveEntityFromSomewhereSlowly("+entityId+")");
                                        try{Thread.sleep(random.nextInt(1000));}
                                        catch(InterruptedException e){System.err.println(e);}
                                        Object entity = new LargeDataStorage() {
                                          // simulate large entity object
                                          private static final int LARGE = 1000;
                                          private int[] storage = new int[LARGE];
                                          { for (int index: storage) { storage[index] = entityId; } }
                                          public int getElement(int index) {return storage[index];}
                                          @Override
                                          public String toString() {return String.valueOf(storage[0]) + " " + LARGE;}
                                        };
                                        DEBUG("< retrieveEntityFromSomewhereSlowly returns "+entity);
                                        return entity;
                                      }
                                    
                                      private void addEntityToCache(int entityId, Object entity) {
                                        DEBUG("| addEntityToCache("+entityId+", "+entity+")");
                                        cache.put(entityId, entity);
                                      }
                                    
                                      public static void main(String[] args)
                                      {
                                        final CorlettsThreadsafeCache cache = new CorlettsThreadsafeCache();
                                        for (int i=0; i<10; i++) {
                                          new Thread(
                                            new Runnable() {
                                              @Override
                                              public void run() {
                                                int i = random.nextInt(4);
                                                Object entity = cache.loadEntity(i);
                                                DEBUG("~ "+i+"-->"+entity);
                                              }
                                            }
                                          , "Thread"+i).start();
                                        }
                                      }
                                    
                                      private static void DEBUG(String message) {
                                        System.err.println(Thread.currentThread().getName()+": "+message);
                                      }
                                    }
                                    Code above looks more clean than GnatsThreadsafeCache - which is [definitely a benefit|http://i32.tinypic.com/wmkpd4.jpg]. But in heavily concurrent environment more complicated way (to lock on the individual entity level) can be justified indeed.

                                    Edited by: gnat on Aug 1, 2009 10:28 AM

                                    Edited by: gnat on Aug 1, 2009 10:41 AM

                                    Edited by: gnat on Aug 1, 2009 11:01 AM
                                    1 2 Previous Next