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
How do you get an iterator over the map? Map does not define an iterator() method (and neither does ConcurrentHashMap). — daniu
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
Hi @daniu , I am using an iterator over the entrySet of the map. — Lazarus
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

Lazarus Lazarus · Accepted Answer · 2020-05-07T19:02:50

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);
    }
}

Java concurrentHashMap iteration

2 Answers