#9 Return values of remove replace broken in NonBlockingHashMap

open
nobody
None
5
2009-10-27
2009-10-27
No

Return value of remove(Object key,Object val) and replace(TypeK key, TypeV oldValue, TypeV newValue ) methods in NonBlockingHashMap is currently broken. It uses reference equality (== operator) check instead of identity check (equals method)

Here's a simple test to illustrate the problem:
public void testRemoveReplace() {
ConcurrentMap map = new NonBlockingHashMap();

String key = new String("key");
String value = new String("value");
assertNull(map.put(key, value));

String equalValue = new String("value"); // not identical but equals to old value
String newValue = new String("newValue");
assertTrue(map.replace(key, equalValue, newValue)); // NBHM returns false

equalValue = new String("newValue"); // not identical but equals to old value
assertTrue(map.remove(key, equalValue)); // NBHM returns false

}

Using equals() instead of == solves the problem in both remove() and replace():
public boolean remove ( Object key,Object val ) { return val.equals(putIfMatch( key,TOMBSTONE, val )); }
public boolean replace ( TypeK key, TypeV oldValue, TypeV newValue ) {
return oldValue.equals(putIfMatch( key, newValue, oldValue ));
}

Discussion

  • Alex Miller
    Alex Miller
    2009-10-27

    An example unit test that exposes the remove() bug:

    public void testRemoveIsEqualityBased() {
    ConcurrentMap m = new NonBlockingHashMap();

    // load entry
    String key = new String("key");
    String value = new String("val");
    assertNull(m.put(key, value));

    // remove entry if value exists
    String equalValue = new String("val"); // not identical but equals
    assertTrue(m.remove(key, equalValue)); // NBHM returns false
    }