Java LinkedHashMap with removeEldestEntry causes j

Question

The error looks like this

Exception in thread "Thread-1" java.lang.NullPointerException
    at java.util.LinkedHashMap$Entry.remove(LinkedHashMap.java:332)
    at java.util.LinkedHashMap$Entry.recordAccess(LinkedHashMap.java:356)
    at java.util.LinkedHashMap.get(LinkedHashMap.java:304)
    at Server.getLastFinishedCommands(Server.java:9086)
    at Server.processPacket(Server.java:484)
    at PacketWorker.run(PacketWorker.java:34)
    at java.lang.Thread.run(Thread.java:744)

Inside getLastFinishedCommands I use

   public List<CCommand> getLastFinishedCommands(UserProfile player) {
        List<CCommand> returnList = new ArrayList<CCommand>();

        if(!finishedCommands.containsKey(player.myWebsitecmd-1)) {
            getSavedState(player);
            return null;
        }

        try { //<-- added this try/catch so it doesn't happen again.
            //Get commands.
            CCommand cmd;
            long i;
            long startIndex = player.myWebsitecmd;
            long endIndex = startIndex+LIMIT_COMMANDS;

            for(i = startIndex; i <= endIndex; i++) {
                cmd = finishedCommands.get(i);   //<-- this is line 9086
                if(cmd == null) {
                    return returnList;
                }
                returnList.add(cmd);
            }
        } catch(Exception e) {} //<-- added this try/catch so it doesn't happen again.
        return returnList;
    }

I wanted to make a Map that auto removes old entries so I used this snippet

public static <K, V> Map<K, V> createLRUMap(final int maxEntries) {
    return new LinkedHashMap<K, V>(maxEntries*3/2, 0.7f, true) {
        @Override
        protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
            return size() > maxEntries;
        }
    };
}

Used it like this

public static int final MAX_COMMANDS_QUEUE = 5000;
public Map<Long, CCommand> finishedCommands = createLRUMap(MAX_COMMANDS_QUEUE);

Obviously it's some kind of CocurrentModifcationException which happens when using with multiple threads.. but why does it crash internally, anyone know how I can use this with like a CocurrentHashMap? I'm trying to fix this without resorting to just putting a try/catch around the whole getLastFinishedCommands function.

I want a Map that clears itself from old junk but still holds atleast 5000 key/value entries.

Answer 1

Based on the stacktrace, I assume that the code tries to remove the value from an index whose item has been already removed by another thread. This makes it to throw NPE while accessing the properties of a null reference. Probably, you should try synchronizing the collection

From the documentation of LinkedHashMap

Note that this implementation is not synchronized. If multiple threads access a linked hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally. This is typically accomplished by synchronizing on some object that naturally encapsulates the map. If no such object exists, the map should be "wrapped" using the Collections.synchronizedMap method. This is best done at creation time, to prevent accidental unsynchronized access to the map:

   Map m = Collections.synchronizedMap(new LinkedHashMap(...));

Answer 2

You said that multiple threads are accessing this map. This could indeed cause the NPE in the remove operation of a LinkedHashMap.Entry instance. This is the implementation of this method:

private void remove() {
    before.after = after;
    after.before = before;
}

Here before and after refer to the linked predecessor and successor of the current entry. If another thread already changed the linking between the entries, this could of course result in an unexpected behavior, such as the NPE.

The solution is - you guessed correctly - to wrap your produced map in a synchronized map. Such as:

public static <K, V> Map<K, V> createLRUMap(final int maxEntries) {
    Map<K,V> result = new LinkedHashMap<K, V>(maxEntries*3/2, 0.7f, true) {
        @Override
        protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
            return size() > maxEntries;
        }
    };
    return Collections.synchronizedMap(result);
}

This synchronized wrapper will indeed synchronize all calls to the underlying map, so only one single thread is allowed to go through each method (such as get, put, contains, size, and so on).