Incorrect usage of WeakHashMap

Introduction

At first I wanted to write a post about an elegant design containing a flyweight pattern together with a use of WeakHashMap class for creating a cache, that was done in one of my previous work places, but while writing this I realized that the above mentioned use of a WeakHashMap had a serious failure.

WeakHashMap

So before the running with illustration of the incorrect usage of the class, let explain what WeakHashMap is and where we can apply it. I cut some most appropriate quotes from Oracle Java documentation to resume its essence and the purpose of this Java class:

WeakHashMap - Hash table based implementation of the Map interface, with weak keys. An entry in a WeakHashMap will automatically be removed when its key is no longer in ordinary use. More precisely, the presence of a mapping for a given key will not prevent the key from being discarded by the garbage collector, that is, made finalizable, finalized, and then reclaimed. When a key has been discarded its entry is effectively removed from the map, so this class behaves somewhat differently from other Map implementations.
The behavior of the WeakHashMap class depends in part upon the actions of the garbage collector, so several familiar (though not required) Map invariants do not hold for this class. Because the garbage collector may discard keys at any time, a WeakHashMap may behave as though an unknown thread is silently removing entries.
Each key object in a WeakHashMap is stored indirectly as the referent of a weak reference. Therefore a key will automatically be removed only after the weak references to it, both inside and outside of the map, have been cleared by the garbage collector.

Or in my own words that means, if there is an object A and there is an instance of WeakHashMap B, and A after its creation was put into the B as a key and after a while A becomes to be eligible for garbage collection, then the entry with the key A is as well eligible for garbage collector. For example:

public class WeakMapTest2 {

 public static void main(String a[]) {
  Map<String, Object> weakMap = new WeakHashMap<>();
  List<String> list = new ArrayList<>(); 
  String x = new String("x");  
  String x1 = new String("x1"); 
  String x2 = new String("x2"); 
  weakMap.put(x, new Object());
  weakMap.put(x1, new Object());
  weakMap.put(x2, new Object());
  // x will have strong reference to list 
  list.add(x);
  System.out.println("Map size :" + weakMap.size());
  // force all keys be eligible
  x=x1=x2=null;
  // call garbage collector 
  System.gc();
  try {
   Thread.sleep(100);
  } catch (InterruptedException e) {
   e.printStackTrace();
  }
  
  System.out.println("Map size :" + weakMap.size());
  System.out.println("Map :" + weakMap);
  System.out.println("List :" + list);
 }

}

Output:

Map size before gc :3
Map size after gc :1
Map :{x=java.lang.Object@5437086a}
List :[x]

Resume:

All entries were discarded from the map, except the entry with key x because x had a strong reference to the list.

WeakHashMap as a cache

So as I said in the introduction WeakHashMap can be applied as a cache that keeps entries while they are useful and remove them when they are useless.

public class WeakCache {

 WeakHashMap<Object, Object> conditions = new WeakHashMap<Object, Object>();

 public synchronized Object get(Object condition) {
  if (condition == null)
   throw new NullPointerException("condition");
  Object cachedCondition = conditions.get(condition);
  if (cachedCondition == null) {
   cachedCondition = condition;
   conditions.put(cachedCondition, cachedCondition);

  }
  return cachedCondition;
 }

 public int getSize() {
  return conditions.size();
 }

}

Test:

public class WeakMapTest {

 WeakCache cache = new WeakCache();
 List<Object> list = new ArrayList<>();

 public WeakMapTest() {

 }

 private void initWeakMap() {
  String x = new String("x");  
  String x1 = new String("x1"); 
  String x2 = new String("x2"); 
  cache.get(x1);
  cache.get(x);
  cache.get(x2);  
  list.add(x);
  
 }

 private void clearGC() {
  System.gc();
  try {
   Thread.sleep(100);
  } catch (InterruptedException e) {
   e.printStackTrace();
  }
  
 }

 public static void main(String a[]) {
  WeakMapTest wmt = new WeakMapTest();
  wmt.initWeakMap();
  System.out.println("map before gc: "+wmt.cache.conditions);
  wmt.clearGC();
  System.out.println("map after gc: "+wmt.cache.conditions);
 }

}

After running the class WeakMapTest I was unpleasantly surprised to get the following output:

map before gc: {x=x, x1=x1, x2=x2}
map after gc: {x=x, x1=x1, x2=x2}

That is, the garbage collector didn't do its work, but why? After I've changed the line of WeakCache class from
conditions.put(cachedCondition, cachedCondition);
to
conditions.put(cachedCondition, new Object());
the program returned to work as expected.

So why the bug is occurring? The explanation is that only a key of WeakHashMap has weak references, but a value of WeakHashMap has strong reference, therefore all these objects x, x1, x2 remained not to be  eligible. Thereby the line conditions.put(cachedCondition, cachedCondition)
that assigns the same object, that is the same reference to the value  and to the key, as a matter of fact neutralizes the special trait of WeakHashMap and turns it to function as regular HashMap.

It is funny to say that the code has been running in production environment of that company over 5 years and only my willing to write a post about their great design caused me to figure out the bug.

Fixing the cache

So how anyway, we should to code the WeakCache class in order to benefit from weak references?
For my opinion it seems that the only solution in that case to get both discarding of useless objects and flyweight features ( I'll write about this in the next post) is to replace the problem line
conditions.put(cachedCondition, cachedCondition);
to
conditions.put(cachedCondition, new WeakReference(cachedCondition));

public class WeakCache {

 WeakHashMap<Object, Object> conditions = new WeakHashMap<Object, Object>();
   
 public synchronized Object get(Object condition) {
  if (condition == null)
   throw new NullPointerException("condition");
  Object cachedCondition = conditions.get(condition);
  if (cachedCondition == null) {
   cachedCondition = condition;   
   conditions.put(cachedCondition, new WeakReference<Object>(cachedCondition));
  }
  else{
   @SuppressWarnings("unchecked")
   WeakReference<Object> weakReference = (WeakReference<Object>)cachedCondition;
   cachedCondition = weakReference.get();
  }
  return cachedCondition;
 }

 public int getSize() {
  return conditions.size();
 }

}

Or you can see the generic version of the former code in the my other post Writing generic class.

In the following illustration, we can see that x is still alive because it has strong references with the List, while x1 was discarded because it doesn't have more  strong references.

Weak and Strong references