Bug in java.util.EnumMap.clone()

May 08, 2009 By: erik Category: Complaining, Geeky, Programming 1,218 views

Rate this post:
1 Star2 Stars3 Stars4 Stars5 Stars (No Ratings Yet)
Loading...

At work today we found ourselves totally confused by the behavior of one of our EnumMap instances. No matter what we did to it (setting new values, clearing, etc.), when we iterated through the entry set, the values were the same. After investigating the source code for java.util.EnumMap, it became clear that the problem was based on the fact that the EnumMap instance we were working with had been generated by calling EnumMap.clone() rather than using the constructor. I’ve written a test program to demonstrate this bug. It should NOT behave like this!

The Code


public class EnumMapBug
{
  private static enum E { A,B,C }

  public static void main(final String[] args)
  {
    final EnumMap<E ,String> map =
        new EnumMap<E, String>(E.class);
    map.put(E.A, "apple");
    map.put(E.B, "ball");
    map.put(E.C, "cat");

    // call entrySet() to generate internal entrySet
    map.entrySet();

    // print original values
    System.out.println("\nOriginal values:");
    System.out.println("\nA is for " + map.get(E.A));
    System.out.println("Entries: " + map.entrySet());

    // clone the map.
    System.out.println("\nCloning...now using clone.");
    final EnumMap<E, String> clone = map.clone();

    // from now on, any time we access the clone's entry set,
    // it will WRONGLY give us the original map's entry set.
    System.out.println("\nA is for " + clone.get(E.A));
    System.out.println("Entries: " + clone.entrySet());

    // no matter what we do to the clone's values,
    // the clone's entry set never changes.
    System.out.println("\nSetting new A value...");
    clone.put(E.A, "aardvark");

    // but get() still works
    System.out.println("\nA is for " + clone.get(E.A));
    System.out.println("Entries: " + clone.entrySet());

    // clear the clone
    System.out.println("\nClearing...");
    clone.clear();

    // get() properly returns null
    System.out.println("\nA is for " + clone.get(E.A));
    // but the entry set still contains our original entries
    System.out.println("Entries: " + clone.entrySet());

    // rather amazingly, when we modify the original
    // map's contents, the entry set for the clone changes
    System.out.println("\nSetting new A value...");
    map.put(E.A, "algorithm");

    System.out.println("\nA is for " + clone.get(E.A));
    System.out.println("Entries: " + clone.entrySet());
  }
}

There is a more easily copied and pasted version of this code here.

The Output

The code above results in the following output:

Original values:

A is for apple
Entries: [A=apple, B=ball, C=cat]

Cloning...now using clone.

A is for apple
Entries: [A=apple, B=ball, C=cat]

Setting new A value...

A is for aardvark
Entries: [A=apple, B=ball, C=cat]

Clearing...

A is for null
Entries: [A=apple, B=ball, C=cat]

Updating original map...

A is for null
Entries: [A=algorithm, B=ball, C=cat]

Explanation

When I said “rather amazingly” in the comment, it’s only really amazing when you don’t understand what is going on. When you clone an EnumMap, because of the entry set caching and the bug in clone(), the cloned instance’s entrySet() method will always give you a reference to the original map’s entry set, never to its own. The version of EnumMap that I’m using contains the javadoc info “@version 1.22, 05/05/07”. It’s clear enough to see the bug in clone(). The EnumMap clone() method is:

public EnumMap<K, V> clone() {
  EnumMap<K, V> result = null;
  try {
    result = (EnumMap<K, V>) super.clone();
  } catch(CloneNotSupportedException e) {
    throw new AssertionError();
  }
  result.vals = (Object[]) result.vals.clone();
  return result;
}

But it needs one more line:

public EnumMap<K, V> clone() {
  EnumMap<K, V> result = null;
  try {
    result = (EnumMap<K, V>) super.clone();
  } catch(CloneNotSupportedException e) {
    throw new AssertionError();
  }
  result.vals = (Object[]) result.vals.clone();
  result.entrySet = null;
  return result;
}

This will force regeneration and caching of the entry set in the clone.

I’m rather surprised that I cannot find any reference to this bug on the internet. It’s such an easy fix! The workaround is to never use EnumMap.clone(), but to construct a new one with its copy constructor. Like so, with the key and value in my example code:

final EnumMap<E, String> clone = new EnumMap<E, String>(map);

 
  • Paul

    You’re such a geek. You make me proud.

  • This is actually a pretty big deal. It’s kind of a rare usage of maps, but Java is a very mature language, so it’s pretty special to find a real bug that no one else (that I can find) has written about on the internet before.

    I’ve submitted the bug to Sun. We’ll see if it gets fixed (or any attention at all).

  • You might call it a bug, but actually the behavior of clone is specified as follows in the javadoc for clone in of EnumMap:

    “Returns a shallow copy of this enum map. (The values themselves are not cloned.”

    … and most sources writing about it (i.e. bloch in effective java) tell you to _not_ rely on collections’ clone methods but manually clone each element in the collection.

  • p3t0r, you’re clearly misunderstanding the bug I’m demonstrating here. It has nothing to do with deep copying of the elements. The problem is that the map returned from EnumMap.clone() breaks the Map contract by always returning the entry set of another map, an entry set that never changes despite any calls to put() or clear() on the clone. That, my friend, is seriously broken.

  • That great. I test HashMap clear value and key.
    But EnumMap not clear key. Code of both class is not same