Thread safety with ConcurrentHashMap

[R Naijat]

Hi all,
I have the following class. I use ConcurrentHashMap. I have many threads writing to the maps and a Timer that saves the data in the map every 5 minutes. I manage to achieve thread safety by using putIfAbsent() when I write entries in the map. However, when I read from it and then remove all entries by clear() method, I want no other thread writes to map while I’m in the process of reading the map contents and then removing them. Obviously my code is not threadsafe even with synchronized(lock){}, b/c the thread that owns the lock in saveEntries(), is not necessarily the same thread that writes into my maps in log() method! Unless I lock the whole code in log() with the same lock object!

I was wondering is there any other way to achieve thread safety w/o enforcing synchronizing by an external lock? Any help is greatly appreciated.

public class Logging {
	
	private static Logging instance;	
	private static final String vendor1 = "vendor1";
	private static final String vendor2 = "vendor2";	
	private static long delay = 5 * 60 * 1000;
		
	private ConcurrentMap<String, Event> vendor1Calls = new ConcurrentHashMap<String, Event>();
	private ConcurrentMap<String, Event> vendor2Calls = new ConcurrentHashMap<String, Event>();
	
	private Timer timer;
	
	private final Object lock = new Object();
 
	private Logging(){
		timer = new Timer();
				
		timer.schedule(new TimerTask() {
			public void run() {
				try {
					saveEntries();
				} catch (Throwable t) {
					timer.cancel();
					timer.purge();
				}
			}		
		}, 0, delay);
	}
	  
	public static synchronized Logging getInstance(){	  
	    if (instance == null){
	    	instance = new Logging();
	    }
	    return instance;
	 }
	
public void log(){		
		ConcurrentMap<String, Event> map;
		String key = "";		
				
		if (vendor1.equalsIgnoreCase(engine)){
			map = vendor1Calls;
		}else if(vendor2.equalsIgnoreCase(engine)){  
			map = vendor2Calls;
		}else{
			return;
		}		
		
		
		key = service + "." + method;
// It would be the code if I use a regular HashMap instead of ConcurrentHashMap
		/*Event event = map.get(key);		
		
		// Map does not contain this service.method, create an Event for the first time.
		if(event == null){
			event = new Event();			
			map.put(key, event);
			
			// Map already contains this key, just adjust the numbers.
		}else{
			// Modify the object fields
		}*/
		//}
				
		// Make it thread-safe using CHM
		Event newEvent = new Event();
		Event existingEvent= map.putIfAbsent(key, newEvent); 
		
		if(existingEvent!=null && existingEvent!=newEvent){
			// Modify the object fields
	}
	
/* From Java docs: For aggregate operations such as putAll and clear, concurrent reads may reflect insertion or removal of only some 
 * entries.
 * */	
	
	private void saveEntries(){
		
		Map<String, List<Event>> engineCalls = null;
		try {			
			
			engineCalls = new HashMap<String, List<Event>>();
			List<Event> events = null;
			
// How can I achieve therad safety here w/o applying any lock?
			//synchronized(lock){
				if(!vendor1Calls.isEmpty()){
					events = new ArrayList<Event>();
					events.addAll(vendor1Calls.values());
					engineCalls.put(vendor1, events);
					vendor1Calls.clear();
				}
				if(!vendor2Calls.isEmpty()){
					events = new ArrayList<Event>();
					events.addAll(vendor2Calls.values());
					engineCalls.put(vendor2, events);
					vendor2Calls.clear();
				}
			//}
				
// logICalls() saves the events in the DB.			
			DBHandle.logCalls(engineCalls);
		} catch (Throwable t) {			
		} finally {
			if(engineCalls!=null){
				engineCalls.clear();
			}						
		}	
	}		
}

[Steve Luke]

I haven't really looked at your code, but something you could think of doing is replacing the Map with a new one when it is time to write it out. Make the reference to the map private and don't let it escape. When it is time to save the results, copy the reference to a new local variable, create a new Map and assign it to the original variable. Then use the local variable to save contents. Meanwhile any new entries would be going into the new map and would not interfere with saving.

This only works if the Map you are using is held internal to a class and you can prevent it from being referenced outside that class.


an alternative, of you can't ensure the map doesn't escape, it to create a snapshot copy of the map, save the contents of the copy, then use original.remove(key,value) for each entry in the copied map to safely remove it from the original. This is more work but should be safe without extra locking.