org.­springframework.­util.­ConcurrentReferenceHashMap is not thread-safe

November 28, 2017

The class org.springframework.util.ConcurrentReferenceHashMap which is part of the spring framework is not thread-safe. We see this in the following junit test:

@RunWith(ConcurrentTestRunner.class)
public class TestSpringConcurrentReferenceHashMap {
	private ConcurrentReferenceHashMap map = new ConcurrentReferenceHashMap();
	private KeyAndValue[] keyAndValue;
	private static final int SIZE = 100;
	public TestSpringConcurrentReferenceHashMap()
	{
		keyAndValue = new KeyAndValue[SIZE];	
		for(int i = 0 ; i< SIZE ; i++)
		{
			keyAndValue[i] = new KeyAndValue(i);
		}
	}
	@Test
	public void test()
	{
		for(int i = 0 ; i < SIZE ; i++)
		{
			AtomicInteger result =  (AtomicInteger)map.get(i);	
			if(  result == null )
			{
				 result = (AtomicInteger) map.putIfAbsent(  keyAndValue[i].key  , keyAndValue[i].value  );
			}	
			if( result != null )
			{
				result.addAndGet(1);
			}
		}
	}
	class KeyAndValue
	{
		KeyAndValue(int key)
		{
			this.key = key;
			value= new AtomicInteger();
		}
		Integer key;
		AtomicInteger value;
	}
}

The ConcurrentTestRunner runs the test by 4 threads in parallel. vmlens, a tool to detect deadlocks and race conditions during the test run, shows the following race conditions:

For example, the first race is the access to a value of an array without correct synchronization. In the method getReference the value of the array is read in line 12

@Nullable
		public Reference<K, V> getReference(@Nullable Object key, int hash, Restructure restructure) {
			if (restructure == Restructure.WHEN_NECESSARY) {
				restructureIfNecessary(false);
			}
			if (this.count == 0) {
				return null;
			}
			// Use a local copy to protect against other threads writing
			Reference<K, V>[] references = this.references;
			int index = getIndex(hash, references);
			Reference<K, V> head = references[index];
			return findInChain(head, key, hash);
		}

And in the method add the value of the array is written in line 22.

		@Nullable
		public <T> T doTask(final int hash, final Object key, final Task<T> task) {
			boolean resize = task.hasOption(TaskOption.RESIZE);
			if (task.hasOption(TaskOption.RESTRUCTURE_BEFORE)) {
				restructureIfNecessary(resize);
			}
			if (task.hasOption(TaskOption.SKIP_IF_EMPTY) && this.count == 0) {
				return task.execute(null, null, null);
			}
			lock();
			try {
				final int index = getIndex(hash, this.references);
				final Reference<K, V> head = this.references[index];
				Reference<K, V> reference = findInChain(head, key, hash);
				Entry<K, V> entry = (reference != null ? reference.get() : null);
				Entries entries = new Entries() {
					@Override
					public void add(V value) {
						@SuppressWarnings("unchecked")
						Entry<K, V> newEntry = new Entry<>((K) key, value);
						Reference<K, V> newReference = Segment.this.referenceManager.createReference(newEntry, hash, head);
						Segment.this.references[index] = newReference;
						Segment.this.count++;
					}
				};
				return task.execute(reference, entry, entries);
			}
			finally {
				unlock();
				if (task.hasOption(TaskOption.RESTRUCTURE_AFTER)) {
					restructureIfNecessary(resize);
				}
			}
		}

The field references is declared as volatile. But the access to the value of the array references[index] is not volatile. To make the access to an array volatile we need to use java.util.concurrent.atomic.AtomicReferenceArray.

The consequence of the race conditions

To see the consequence of the race conditions we use jcstress, an open JDK code tool: The Java Concurrency Stress tests (jcstress) is an experimental harness and a suite of tests to aid the research in the correctness of concurrency support in the JVM, class libraries, and hardware.

I use the following test class:

@JCStressTest
@Outcome(id = "10", expect = Expect.ACCEPTABLE, desc = "Default outcome.")
@State
public class SpringConcurrentHashMap {
	private final ConcurrentReferenceHashMap map = new ConcurrentReferenceHashMap();
	private final KeyAndValue[] keyAndValue;
	private final int SIZE = 10;
	class KeyAndValue {
		KeyAndValue(int key) {
			this.key = key;
			value = new AtomicInteger();
		}
		final Integer key;
		final AtomicInteger value;
	}
	public SpringConcurrentHashMap() {
		keyAndValue = new KeyAndValue[SIZE];
		for (int i = 0; i < SIZE; i++) {
			keyAndValue[i] = new KeyAndValue(i);
		}
	}
	public void test() {
		for (int i = 0; i < SIZE; i++) {
			AtomicInteger result = (AtomicInteger) map.get(i);
			if (result == null) {
				result = (AtomicInteger) map.putIfAbsent(keyAndValue[i].key, keyAndValue[i].value);
			}
			if (result != null) {
				result.addAndGet(1);
			}
		}
	}
	@Actor
	public void actor1() {
		test();
	}
	@Actor
	public void actor2() {
		test();
	}
	@Arbiter
	public void arbiter(IntResult1 r) {
		int sum = 0;
		for (int i = 0; i < SIZE; i++) {
			sum += ((AtomicInteger) map.get(i)).get();
		}
		r.r1 = sum;
	}
}

The jcstress tool runs each actor in a separate thread, repeating the test multiple times. We have two actor method, each creating a new AtomicInteger in the hash map if no value is there or incrementing the AtomicInteger if a value existed. Therefore we expect the sum for all values in the hash map after the thread was run 10. If I run this test using the stress mode on my development machine, an intel i5 4 core CPU with oracle JDK 8, I see the following result:

   2 matching test results. 
  [FAILED] com.vmlens.stressTest.tests.SpringConcurrentHashMap
    (JVM args: [-client])
  Observed state   Occurrences   Expectation  Interpretation                                              
              10    92,597,143    ACCEPTABLE  Default outcome.                                            
               9             7     FORBIDDEN  No default case provided, assume FORBIDDEN                  
  [FAILED] com.vmlens.stressTest.tests.SpringConcurrentHashMap
    (JVM args: [-server])
  Observed state   Occurrences   Expectation  Interpretation                                              
              10    98,517,611    ACCEPTABLE  Default outcome.                                            
               9             9     FORBIDDEN  No default case provided, assume FORBIDDEN                  

In a small percentage of all cases, 7 cases for -client and 9 cases for -server the count is less than 10. So in some cases, a value in the map is overridden by a new value.

Make your application thread safe

LEARN MORE