Java concurrentHashMap iteration

Question

I am using a Thread (Let's call it "MapChecker") which is looping during its entire lifetime on a ConcurrentHashMap.

The map is populated from other threads and its cleared by the MapChecker by using iterators over it.

The map has the following structure:

private volatile Map<MyObject, SynchronizedList<MyOtherObject>> map = new ConcurrentHashMap<>(); //SynchronizedList = Collections.syncrhonizedList.

MapChecker must update the values of each key inside its loop. Updates are made by removing elements from the lists or removing the full map entry.

Synchronization occurs on two steps:

When adding data inside the map (synchronized)
When retrieving the iterator of the map (synchronized inside MapChecker).

The locks are on the mapitself ( synchronized(map) ).

I don't care to have always the last updated values inside my iterator view, but i need to be sure that all the missing values will be retrieved in the next iterations, this is important because I do not want to skip any element. Furthermore, it's important to have also the SynchronizedList correctly updated.

My question is: Can i be sure to get all the entries inserted / updated by having this kind of architecture? Is there the risk to miss something? What happens if MapChecker deletes an entry while an other thread is updating the same entry? ConcurrentHashMap should block these operation so i'm expecting no troubles.

This is the MapChecker loop:

while (!isInterrupted()) { executeClearingPhases(); Iterator<Map.Entry<PoolManager, List<PooledObject>>> it = null; synchronized (idleInstancesMap) { it = idleInstancesMap.entrySet().iterator(); } while (it.hasNext()) { Map.Entry<PoolManager, List<PooledObject>> entry = it.next(); PoolManager poolManager = entry.getKey(); boolean stop = false; while (!stop) { //this list is empty very often but it shouldn't, that's the problem I am facing. I need to assure updates visibility. List<PooledObject> idlePooledObjects = entry.getValue(); if (idlePooledObjects.isEmpty()) { stop = true; } else { PooledObject pooledObject = null; try { pooledObject = idlePooledObjects.get(0); info(loggingId, " - REMOOOVINNGG: \"", pooledObject.getClientId(), "\"."); PoolingStatus destroyStatus = poolManager.destroyIfExpired(pooledObject); switch (destroyStatus) { case DESTROY: info(loggingId, " - Removed pooled object \"", pooledObject.getClientId(), "\" from pool: \"", poolManager.getClientId(), "\"."); idlePooledObjects.remove(0); break; case IDLE: stop = true; break; default: idlePooledObjects.remove(0); break; } } catch (@SuppressWarnings("unused") PoolDestroyedException e) { warn(loggingId, " - WARNING: Pooled object \"", pooledObject.getClientId(), "\" skipped, pool: \"", poolManager.getClientId(), "\" has been destroyed."); synchronized(idleInstancesMap) { it.remove(); } stop = true; } catch (PoolManagementException e) { error(e, loggingId, " - ERROR: Errors occurred during the operation."); idlePooledObjects.remove(0); } } } } Thread.yield(); }

This is the method invoked (many times) by any other thread:

public void addPooledObject(PoolManager poolManager, PooledObject pooledObject) { synchronized (idleInstancesMap) { List<PooledObject> idleInstances = idleInstancesMap.get(poolManager); if (idleInstances == null) { idleInstances = Collections.synchronizedList(new LinkedList<PooledObject>()); idleInstancesMap.put(poolManager, idleInstances); } idleInstances.add(pooledObject); } }

Thanks

Why is the map itself volatile? Cannot you make it final ? — Thilo
– Thilo, Commented May 7, 2020 at 12:48
How do you get an iterator over the map? Map does not define an iterator() method (and neither does ConcurrentHashMap). — daniu
– daniu, Commented May 7, 2020 at 12:58
synchronized "When retrieving the iterator of the map ". Do you mean the whole iteration is in the critical section? Or just retrieving the iterator itself ? — Thilo
– Thilo, Commented May 7, 2020 at 12:58
Hi @daniu , I am using an iterator over the entrySet of the map. — Lazarus
– Lazarus, Commented May 7, 2020 at 13:02
The point is, the fact that you declared the variable map as volatile indicates that you probably don’t understand what this modifier does. The variable map should not change and it is best to demonstrate that it doesn’t by declaring the variable final. — Holger
– Holger, Commented May 7, 2020 at 14:01

Lazarus · Accepted Answer · 2020-05-07 19:02:50Z

Thanks to PatrickChen's suggestion I moved the list of PooledObject instances inside each PoolManager (which already owns this list since it owns the pool and its internal state in a fully synchronized manner).

This is the result:

//MapChecker lifecycle public void run() { try { while (!isInterrupted()) { executeClearingPhases(); ListIterator<PoolManager> it = null; //This really helps. poolManagers is the list of PoolManager instances. //It's unlikely that this list will have many elements (maybe not more than 20) synchronized (poolManagers) { Iterator<PoolManager> originalIt = poolManagers.iterator(); while (originalIt.hasNext()) { if (originalIt.next().isDestroyed()) { originalIt.remove(); } } //This iterator will contain the current view of the list. //It will update on the next iteration. it = new LinkedList<PoolManager>(poolManagers).listIterator(); } while (it.hasNext()) { PoolManager poolManager = it.next(); try { //This method will lock on its internal synchronized pool in order to //scan for expired objects. poolManager.destroyExpired(); } catch (@SuppressWarnings("unused") PoolDestroyedException e) { warn(loggingId, " - WARNING: Pool: \"", poolManager.getClientId(), "\" has been destroyed."); it.remove(); } } Thread.yield(); } throw new InterruptedException(); } catch (@SuppressWarnings("unused") InterruptedException e) { started = false; Thread.currentThread().interrupt(); debug(loggingId, " - Pool checker interrupted."); } } //Method invoked by multiple threads public void addPooledObject(PoolManager poolManager) { synchronized (poolManagers) { poolManagers.add(poolManager); } }

PatrickChen · Accepted Answer · 2020-05-07 13:14:33Z

but I need to be sure that all the missing values will be retrieved in the next iterations, this is important because I do not want to skip any element.

First, I think based on your solution, I think you will get all the items in the map as long as you keep doing the MapChecker loop. I suggest you have an extra while(true) loop outside the MapChecker code you present.

But based on all your description, I suggest you should use Queue instead of Map, obviously, your problem need a push/pop operation, maybe BlockingQueue is a better fit here.

Yes but the problem is that i need an insertion ordered list for each PoolManager object. So using queues is not an option since I may have to add new entries or new elements inside each list.
all the PoolManager generates PooledObject right? why not add the PoolManager information into the PoolObject, then you can just keep one Queue, no Map needed.
Solved! By following your suggestion I changed my data structures and now code is much cleaner and works. Thanks

Collectives™ on Stack Overflow

Java concurrentHashMap iteration

2 Answers 2

Comments

3 Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

3 Comments

Related