Synchronizing on two or more objects (Java)

Question

I have code similar to following:

public class Cache{
 private final Object lock = new Object();
 private HashMap<Integer, TreeMap<Long, Integer>> cache = 
  new HashMap<Integer, TreeMap<Long, Integer>>();
 private AtomicLong FREESPACE = new AtomicLong(102400);

 private void putInCache(TreeMap<Long, Integer> tempMap, int fileNr){
  int length; //holds the length of data in tempMap
  synchronized(lock){
   if(checkFreeSpace(length)){
    cache.get(fileNr).putAll(tmpMap);
    FREESPACE.getAndAdd(-length);
   }
  }
 }

 private boolean checkFreeSpace(int length){      
  while(FREESPACE.get() < length && thereIsSomethingToDelete()){
   // deleteSomething returns the length of deleted data or 0 if 
   // it could not delete anything
   FREESPACE.getAndAdd(deleteSomething(length));
  }
  if(FREESPACE.get() < length) return true;
  return false;
 }
}

putInCache is called by about 139 threads a second. Can I be sure that these two methods will synchronize on both cache and FREESPACE? Also, is checkFreeSpace() multithread-safe i.e can I be sure that there will be only one invocation of this method at a time? Can the "multithread-safety" of this code be improved?

(There is no point using AtomicLong in that code.)

Tom Hawtin - tackline
– Tom Hawtin - tackline

2009-11-27 17:31:52 +00:00
Commented Nov 27, 2009 at 17:31 — Tom Hawtin - tackline
– Tom Hawtin - tackline, Commented Nov 27, 2009 at 17:31

Henry · Accepted Answer · 2009-11-27 11:48:01Z

3

To have your question answered fully, you would need to show the implementations of the thereIsSomethingToDelete() and deleteSomething() methods.

Given that checkFreeSpace is a public method (does it really need to be?), and is unsynchronized, it is possible it could be called by another thread while the synchronized block in the putInCache() method is running. This by itself might not break anything, since it appears that the checkFreeSpace method can only increase the amount of free space, not reduce it.

What would be more serious (and the code sample doesn't allow us to determine this) is if the thereIsSomethingToDelete() and deleteSomething() methods don't properly synchronize their access to the cache object, using the same Object lock as used by putInCache().

answered Nov 27, 2009 at 11:48

Henry

1,4992 gold badges11 silver badges17 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Azimuth Over a year ago

Thanks for your reply. First of all, public is not a big issue because these two methods are only called by another method in the same class. As for thereIsSomethingToDelete() and deleteSomething(), I do not have these method in fact. Their logic is included to checkFreeSpace method but it would be too messy if I would put all code here. So let's just suppose those methods are properly synchronized.

Azimuth Over a year ago

Sorry, I chose wrong methods to ask questions about. Please take a look on edited version.

Henry Over a year ago

So presumably putInCache() is not really private then either (otherwise how could the class actually be used). I think then that the class is threadsafe as described, since all access to mutable state can only occur if a thread holds the lock established by the putInCache() synchronized block.

Martin OConnor · Accepted Answer · 2009-11-27 11:21:13Z

2

You don't usually synchronize on the fields you want to control access to directly. The fields that you want to synchronize access to must only be accessed from within synchronized blocks (on the same object) to be considered thread safe. You are already doing this in putInCache(). Therefore, because checkFreeSpace() accesses shared state in an unsynchronized fashion, it is not thread safe.

answered Nov 27, 2009 at 11:21

Martin OConnor

3,5934 gold badges28 silver badges32 bronze badges

2 Comments

Azimuth Over a year ago

what if I declare putInCache() as synchronized? Will that make both methods "safer"?

Martin OConnor Over a year ago

@Azimuth You need to protect access at the field level, which means all methods that access those fields need to synchronize on the same lock object. In short, you will need to add a synchronized block to the checkFreeSpace() too.

Collectives™ on Stack Overflow

Synchronizing on two or more objects (Java)

2 Answers 2

3 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related