FindBugs: DMI_ENTRY_SETS_MAY_REUSE_ENTRY_OBJECTS

Question

I implemented the very nice sorting solution found here:

static <K,V extends Comparable<? super V>> SortedSet<Map.Entry<K,V>>
entriesSortedByValues(Map<K,V> map) {

    SortedSet<Map.Entry<K,V>> sortedEntries = new TreeSet<Map.Entry<K,V>>(

        new Comparator<Map.Entry<K,V>>() {
            @Override
            public int compare(Map.Entry<K,V> e1, Map.Entry<K,V> e2) {
                int res = e1.getValue().compareTo(e2.getValue());
                return res != 0 ? res : 1;
            }
        }
    );

    sortedEntries.addAll(map.entrySet());
    return sortedEntries;
}

The code seems to work great. However, FindBugs complains about this line:

sortedEntries.addAll(map.entrySet());

The complaint is:

Bug: Adding elements of an entry set may fail due to reuse of Map.Entry object in com.local.sem.util.MapUtil.entriesSortedByValues(Map)

The entrySet() method is allowed to return a view of the underlying Map in which a single Entry object is reused and returned during the iteration. As of Java 1.6, both IdentityHashMap and EnumMap did so. When iterating through such a Map, the Entry value is only valid until you advance to the next iteration. If, for example, you try to pass such an entrySet to an addAll method, things will go badly wrong.

Confidence: Normal, Rank: Troubling (14)
Pattern: DMI_ENTRY_SETS_MAY_REUSE_ENTRY_OBJECTS
Type: DMI, Category: BAD_PRACTICE (Bad practice)

Can anyone tell me what that means or if it's actually relevant to this particular code?

score 3 · Accepted Answer · answered Aug 08 '12 at 17:59

3

Here's a simple example of the problem:

Map<String,String> map = new IdentityHashMap<String,String>();
map.put("a", "1");
map.put("b", "2");
Iterator<Entry<String,String>> i = map.entrySet().iterator();
Entry<String,String> e1 = i.next();
System.out.println("first key is: " + e1.getKey());
Entry<String,String> e2 = i.next();
System.out.println("first key is now: " + e1.getKey());

Using Java 6, this prints:

first key is: a
first key is now: b

This is because the second call to i.next() returns the same Entry as the first, but it has changed the values stored in that Entry.

If I change the IdentityHashMap to HashMap, each Entry that is returned is different, so e1.getKey() doesn't change.

answered Aug 08 '12 at 17:59

TimK

4,635
2
27
27

Ok. I now fully understand the concept. Thanks for example. However, in practice I haven't found this to be true. I ran your code with HashMap, Hashtable, IdentityHashMap, LinkedHashMap, Properties, TreeMap, and WeakHashMap and each time I got the same value for both outputs. I'm using Java 7. Could that have made the difference? – dhalsim2 Aug 08 '12 at 18:51
Yes, I see the same thing that you do with Java 7. So they must have changed the implementation of entrySet() again in Java 7. – TimK Aug 08 '12 at 19:20
If you look at the source of IdentityHashMap you can see the difference. In Java 6, EntryIterator.next always returns the same object (the iterator itself). In Java 7, EntryIterator.next always returns a next Entry object. – TimK Aug 08 '12 at 20:33
Thanks for the analysis. I re-ran the code in Java 6 and confirmed that the results are different. I now fully understand what FindBugs is complaining about. But for my specific example, I'm not entirely sure what I should do. It looks like I'm safe with Java 7, but if the API spec allows for re-using Map.Entry, ideally I wouldn't want my code to break with some future version of Java or some alternate JVM. Should I just leave the code as-is and hope for the best, or should I modify it in some way to prevent future disaster? – dhalsim2 Aug 08 '12 at 21:04
Perhaps instead of using `addAll`, I should iterate through the `entrySet`, make a defensive copy of each entry using `new AbstractMap.SimpleImmutableEntry(entry.getKey(), entry.getValue())` and then add that copy. – dhalsim2 Aug 08 '12 at 21:07
That seems like the safest thing to do. – TimK Aug 08 '12 at 23:05

score -3 · Answer 2 · answered Aug 08 '12 at 01:56

The entrySet() method is allowed to return a view of the underlying Map in which a single Entry object is reused and returned during the iteration. As of Java 1.6, both IdentityHashMap and EnumMap did so. When iterating through such a Map, the Entry value is only valid until you advance to the next iteration. If, for example, you try to pass such an entrySet to an addAll method, things will go badly wrong.

Please read the link for more details. http://findbugs.sourceforge.net/bugDescriptions.html#DMI_ENTRY_SETS_MAY_REUSE_ENTRY_OBJECTS

It's a very simple and good explanation on findbug site and I thought you did not read it.. — Manisha Mahawar, Aug 08 '12 at 20:21

FindBugs: DMI_ENTRY_SETS_MAY_REUSE_ENTRY_OBJECTS

2 Answers2