Reputation: 177
Java concurrentHashMap iteration
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
Upvotes: 0
Views: 119
Answers (2)
Reputation: 177
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);
}
}
Upvotes: 1
Reputation: 1420
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.
Upvotes: 0