ConcurrentModificationException: removePassengers() silently downgrades ConcurrentHashMap to HashMap

[Lxk4z]

Bug: ConcurrentModificationException in PacketSendListener/PassengerManager under concurrent packet load

Version: PassengerAPI 1.5.1

Description

We're seeing repeated ConcurrentModificationException warnings from PacketEvents when PassengerAPI's listener handles SET_PASSENGERS packet-send events, under moderate concurrent player load (multiple lobby server instances, several dozen concurrent players). PacketEvents catches the exception so the server doesn't crash, but the affected packet send silently fails and the warning floods logs under load (~230 occurrences in a 2-hour window on a single one of our servers).

Stack traces observed

java.util.ConcurrentModificationException
    at java.base/java.util.HashMap.computeIfAbsent(Unknown Source)
    at com.maximde.passengerapi.listeners.PacketSendListener.onPacketSend(PacketSendListener.java:36)
    at com.github.retrooper.packetevents.event.PacketListener$1.onPacketSend(PacketListener.java:46)
    ...

java.util.ConcurrentModificationException
    at java.base/java.util.HashMap.computeIfAbsent(Unknown Source)
    at com.maximde.passengerapi.PassengerManager.addPassengers(PassengerManager.java:117)
    ...

Root cause

PassengerManager.passengersHashmap is declared as a ConcurrentHashMap, and everywhere else in the class the inner maps/sets are correctly created with ConcurrentHashMap/ConcurrentHashMap.newKeySet(), e.g. in addPassengers:

passengersHashmap.computeIfAbsent(PLUGIN_NAME, k -> new ConcurrentHashMap<>())
        .computeIfAbsent(targetEntity, k -> ConcurrentHashMap.newKeySet())
        .addAll(passengerSet);

However, removePassengers(boolean async, int[] passengerIDs, boolean sendPackets) rebuilds the per-plugin map as a plain HashMap (and its value sets as plain HashSets), and then puts that plain map back into passengersHashmap, replacing the previously concurrent-safe entry:

Map<Integer, Set<Integer>> newPluginMap = new HashMap<>(); // not thread-safe
...
Set<Integer> passengers = new HashSet<>(pluginEntry.getValue()); // not thread-safe
...
passengersHashmap.put(entry.getKey(), newPluginMap); // replaces the ConcurrentHashMap entry with a plain HashMap

After this method runs once for a given plugin key, passengersHashmap.get(pluginKey) is silently a non-thread-safe HashMap, even though the outer type still says ConcurrentHashMap<String, Map<Integer, Set<Integer>>>. Any subsequent concurrent access — e.g. addPassengers() being called from a Netty event-loop thread while another thread reads/writes the same map — hits HashMap.computeIfAbsent on this now-unsafe map and throws ConcurrentModificationException.

Suggested fix

Keep the concurrent-safe types when rebuilding the map in removePassengers(int[], boolean):

Map<String, Map<Integer, Set<Integer>>> tempHashmap = new HashMap<>(passengersHashmap);
for (Map.Entry<String, Map<Integer, Set<Integer>>> entry : tempHashmap.entrySet()) {
    Map<Integer, Set<Integer>> pluginMap = entry.getValue();
    Map<Integer, Set<Integer>> newPluginMap = new ConcurrentHashMap<>(); // was: new HashMap<>()

    for (Map.Entry<Integer, Set<Integer>> pluginEntry : pluginMap.entrySet()) {
        int targetEntity = pluginEntry.getKey();
        Set<Integer> passengers = ConcurrentHashMap.newKeySet(); // was: new HashSet<>(...)
        passengers.addAll(pluginEntry.getValue());
        passengers.removeAll(passengerSet);
        if (!passengers.isEmpty()) {
            newPluginMap.put(targetEntity, passengers);
        }
    }

    if (!newPluginMap.isEmpty()) {
        passengersHashmap.put(entry.getKey(), newPluginMap);
    } else {
        passengersHashmap.remove(entry.getKey());
    }
}
if (sendPackets) sendPassengerPackets(false);

Since newPluginMap (and its value sets) become shared state stored back into passengersHashmap and read/written concurrently from packet-event threads, they need to stay concurrent-safe just like everywhere else in this class.

Impact

Low severity (caught by PacketEvents, no crash), but causes silently dropped passenger packets under load and significant log noise. Happy to provide the full OpenSearch log export around the timestamps above if useful.