0

A version of the static method append() below, is sometimes called simultaneously by various threads:

public class CMEDemo {

    private static final Logger LOG = Logger.getLogger(CMEDemo.class.getName());

    private static final String[] SOME_DEMO = {"albino", "mosquito", "libido"};
    private static final Set<String> SET_SOURCE = new LinkedHashSet<>(Arrays.asList(SOME_DEMO));

    public static void append() {
//ConcurrentModificationException is thrown in the constructor for modifiableCopy.
        LinkedHashSet<String> modifiableCopy = new LinkedHashSet<>(getBuiltInFunctions());
        //Exception is not thown here, or later.
        Set<String> doomed = modifiableCopy.stream()
                .filter(f -> f.contains("quit")).collect(Collectors.toSet());
        for (String found : doomed) {
            LOG.log(Level.INFO, found);
        }
    }

    public static Set<String> getBuiltInFunctions() {
        return Collections.unmodifiableSet(SET_SOURCE);
    }

}

Normally, all works as expected, but sometimes the LinkedHashSet constructor throws a ConcurrentModificationException:

java.util.ConcurrentModificationException
        at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:719)
        at java.util.LinkedHashMap$LinkedKeyIterator.next(LinkedHashMap.java:742)
        at java.util.Collections$UnmodifiableCollection$1.next(Collections.java:1042)
        at java.util.AbstractCollection.addAll(AbstractCollection.java:343)
        at java.util.LinkedHashSet.<init>(LinkedHashSet.java:169)

My JVM is Java 1.8_212. If I setup a test case to spawn multiple threads and let it run for a while, append() eventually throws ConcurrentModificationException. Why is this exception thrown, and how do I safely get a LinkedHashSet?

Charlweed
  • 1,517
  • 2
  • 14
  • 22
  • Does any code modify `SET_SOURCE` (ie, by accessing it directly, not via `getBuiltInFunctions()`)? – yshavit Jun 06 '19 at 22:17
  • Yes, that's why I make the copy. – Charlweed Jun 06 '19 at 22:18
  • 1
    `unmodifiableSet` doesn't make a copy. It returns a Set whose modification methods throw UnsupportedOperationException, and whose other methods delegate to the underlying set. If that underlying set changes, then the `unmodifiableSet` will reflect those changes -- which, if you're in the middle of iterating it, will cause this exception. – yshavit Jun 06 '19 at 22:20
  • Just to make sure, you do know that `Collections.unmodifiableSet(..)` doesn't copy the passed collection, right? – Tom Jun 06 '19 at 22:20
  • NO, I DIDN'T I thought it was a copy. Well, that probably explains it. – Charlweed Jun 06 '19 at 22:22
  • If you want a really immutable list, then either use one of Java 11s factories like `List.of(...)` or a library like Guava with provides types like `ImmutableList`. – Tom Jun 06 '19 at 22:24
  • Ugh. All I wanted was to avoid making the exact mistake I made. To fix this, I'll have `getBuiltInFunctions()` return a copy, and and change any other code that accesses the SET_SOURCE Set. – Charlweed Jun 06 '19 at 22:33

1 Answers1

0

Other code was modifying the Set SET_SOURCE which was wrapped, not copied, by Collections.unmodifiableSet(..), and that caused intermittent exceptions during the construction of the copy in LinkedHashSet.

So I replaced getBuiltInFunctions() with the equivalent of

 public static Set<String> getBuiltInFunctions() {
        synchronized (SET_SOURCE) {
            return Collections.unmodifiableSet(new HashSet<>(SET_SOURCE));
        }
    }

    private static Set<String> getFunctionsRW() {
        synchronized (SET_SOURCE ) {
            return SET_SOURCE;
        }
    }
Charlweed
  • 1,517
  • 2
  • 14
  • 22
  • Well, do you still need `Collections.unmodifiableSet(..)`? Having `new HashSet<>(SET_SOURCE)` provides with the security you wanted, right? – Tom Jun 07 '19 at 14:01
  • It was supposed to be a read-only copy, and to force clients to make local modifiable copies. But.next checkpoint I'll review, and reduce the duplication. – Charlweed Jun 07 '19 at 17:29