Spring Weak ConcurrentReferenceHashMap forgets strongly reachable objects after GC

Affects: 5.2.2 and 5.0.12

ConcurrentReferenceHashMap with reference type WEAK have entries disappear on garbage collections even though the keys and values stored in the map have strong references outside of the map. Given enough memory pressure, SOFT entries would probably fail similarly.

I suspect this happens because the ConcurrentReferenceHashMap.WeakEntryReference class (which extends WeakReference<Entry>) is the only thing (weakly) referencing any Entry. The Segment.references items are allWeakReferences. Think you need to refactor the map to directly have WeakReference<K> and WeakReference<V> instances instead of WeakReference<Entry<K,V>.

Soft references appear to be handled the identically, so any fix should be applied to those references as well of course.

Sample failing unit test (fails with AdoptOpenJDK version 8u222, issue was spotted on IBM JDK 8):

package test;

import org.junit.Test;
import org.springframework.util.Assert;
import org.springframework.util.ConcurrentReferenceHashMap;

import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.ConcurrentMap;

public class WeakConcurrentReferenceHashMapTest {
    @Test
    public void testWeakConcurrentReferenceHashMap() {
        final ConcurrentMap<Integer, Integer> map =
                new ConcurrentReferenceHashMap<>(10, ConcurrentReferenceHashMap.ReferenceType.WEAK);
        final List<Integer> doNotGc = new ArrayList<>();
        final Integer key = new Integer(123456);
        final Integer value = new Integer(888888);
        doNotGc.add(key);
        doNotGc.add(value);
        final WeakReference<Integer> weakKey = new WeakReference<>(key);
        final WeakReference<Integer> weakValue = new WeakReference<>(value);

        // put it there
        map.put(key, value);
        Assert.isTrue(Objects.equals(value, map.get(key)), "1st get");

        // wipe map (comment out to make asserts pass)
        System.gc();

        // A weak reference to key or value should still be reachable after the GC
        Assert.notNull(weakKey.get(), "weak key ref");
        Assert.notNull(weakValue.get(), "weak value ref");
        // map.get() returns null
        Assert.isTrue(Objects.equals(value, map.get(key)), "get post-GC");
        Assert.isTrue(doNotGc.size() == 2, "I still remember all");
    }
}

Comment From: sbrannen

Thanks for raising the issue.

@philwebb, what are your thoughts on the matter?

Feel free to use the following JUnit Jupiter version of the test case.

package test;

import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ConcurrentMap;

import org.junit.jupiter.api.Test;

import org.springframework.util.ConcurrentReferenceHashMap;

import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.springframework.util.ConcurrentReferenceHashMap.ReferenceType.WEAK;

class WeakConcurrentReferenceHashMapTest {

    @Test
    void weakReferences() {
        ConcurrentMap<Integer, Integer> map = new ConcurrentReferenceHashMap<>(10, WEAK);
        List<Integer> doNotGc = new ArrayList<>();
        Integer key = new Integer(123456);
        Integer value = new Integer(888888);
        doNotGc.add(key);
        doNotGc.add(value);
        WeakReference<Integer> weakKey = new WeakReference<>(key);
        WeakReference<Integer> weakValue = new WeakReference<>(value);

        // put it there
        map.put(key, value);

        assertEquals(value, map.get(key), "get pre-GC");

        // wipe map (comment out to make failing assertion pass)
        System.gc();

        assertAll(
            // A weak reference to key or value should still be reachable after the GC
            () -> assertNotNull(weakKey.get(), "weak key ref"),
            () -> assertNotNull(weakValue.get(), "weak value ref"),
            // map.get(key) returns null
            () -> assertEquals(value, map.get(key), "get post-GC"),
            () -> assertEquals(2, doNotGc.size(), "I still remember all"));
    }

}

Comment From: stela

Related? https://github.com/spring-cloud/spring-cloud-netflix/issues/2443 https://github.com/spring-cloud/spring-cloud-openfeign/issues/5 https://stackoverflow.com/questions/47289155/how-does-concurrentreferencehashmap-work

Comment From: philwebb

It's been a while since I worked on that code, but the original intention was to create a class that can be used for Spring's internal caches. I don't think we ever consider the scenario described above, and I'm not sure that we should consider it a bug.

I remember it being quite tricky to write the class, and I can't easily see how it could be refactored to use two different references (but I'll happily try to review any code if someone has the appetite to change things).

I personally think we should make this a documentation issue. The class has served quite well for the use-case for which it was designed and I'm not sure changing it at this point is a good idea from a framework stability perspective.

Comment From: stela

In case this will be resolved as a documentation issue and somebody comes here looking for a working-as-previously-documented replacement, you can try the ConcurrentReferenceHashMap implementations in Hibernate or Hibernate-validator (same class name, different package names), or in case you only need weak key/value references (with identity-based equality checks), Guava's new MapMaker().weakKeys().weakValues().makeMap().

Comment From: sbrannen

I personally think we should make this a documentation issue. The class has served quite well for the use-case for which it was designed and I'm not sure changing it at this point is a good idea from a framework stability perspective.

I tend to agree with @philwebb on this point, but I'll assign this the "team attention" label for further input.