Java Concurrency with Sets

Question

For the below function, I am trying to return a new Set mHashSet that is a copy of another set iGraphicSectors:

public Set<String> getGraphics() {
    synchronized (iGraphicSectors) {  // class variable of type Set<String>
        LinkedHashSet<String> mHashSet = new LinkedHashSet<String>();
        synchronized (mHashSet) {
            mHashSet.addAll(iGraphicSectors);
        }
        return mHashSet;
    }
}

However line 5 mHashSet.addAll(iGraphicSectors); is throwing a ConcurrentModificationException (I am not sure how this is possible). Is there a way I can accomplish the above task in a thread-safe manner?

Yes, either change all code using iGraphicSectors to wrap it in a synchronized (iGraphicSectors), or change iGraphicSectors to be class that is thread safe for iteration. — Andreas
– Andreas, Commented Jan 12, 2017 at 18:05
Synchronizing on mHashSet is unnecessary. There is no way, that any other thread could have a reference to that newly created set instance. — Seelenvirtuose
– Seelenvirtuose, Commented Jan 12, 2017 at 18:06
Side note: Since you just created mHashSet, and haven't shared it with anyone, there is no reason whatsoever for using synchronized (mHashSet). — Andreas
– Andreas, Commented Jan 12, 2017 at 18:06
You should try to write a small test case that actually reproduces the issue. I don't think you have identified the cause of the CME correctly. — Amir Afghani
– Amir Afghani, Commented Jan 12, 2017 at 18:11
Synchronizing on mHashSet accomplishes nothing because no other thread can possibly access the new object before your method returns. — Solomon Slow
– Solomon Slow, Commented Jan 12, 2017 at 18:27

Nicolas Filotto · Accepted Answer · 2017-01-12 18:55:28Z

1

What you need to do is to use a Set that is thread-safe for iGraphicSectors as you obviously read and modify it concurrently, the simplest way can be to use the decorator Collections.synchronizedSet(Set<T> s) to make your current Set thread-safe, any read and write accesses will then be automatically protected with a synchronized block thanks to the decorator, however you still need to protect iterations over it explicitly with a synchronized block.

Here is an example of how to create it:

Set<String> iGraphicSectors = Collections.synchronizedSet(new HashSet<String>());

Here is how your code would then look like:

public Set<String> getGraphics() {
    // Still needed as the constructor of LinkedHashSet will iterate
    // over iGraphicSectors 
    synchronized (iGraphicSectors) {  
        return new LinkedHashSet<String>(iGraphicSectors);
    }
}

edited Jan 12, 2017 at 18:55

answered Jan 12, 2017 at 18:23

Nicolas Filotto

45.3k11 gold badges97 silver badges129 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

JK Sung Over a year ago

Thank you, I did not know there was also a concurrency wrapper for Sets, I will try this. Although probably not necessary, just to be safe I may also add the below suggestion and return an unmodifiable mHashset.

Nicolas Filotto Over a year ago

I'm afraid that it is not enough, you would move the problem outside your class, if you iterate over Collections.unmodifiableSet(iGraphicSectors) you can still get ConcurrentModificationException as it will still use the iterator from iGraphicSectors. If iGraphicSectors is read and modified concurrently, it must be thread-safe, it is the rule.

Klitos Kyriacou Over a year ago

It's ok to return Collections.unModifiableSet(new LinkedHashSet<>(iGraphicSectors)); just for extra safety from accidentally modifying that set, but that has nothing to do with concurrency and you still have to return a complete copy of the set anyway.

JK Sung Over a year ago

Do I have to put in the individual iGraphicSectors.add() operations into synchronized blocks? i.e. synchronized (iGraphicSectors) { iGraphicSectors.add(graphic); }

Nicolas Filotto Over a year ago

No as it will be done internally by the decorator, only iterations over your set still need to be explicitly protected, check the javadoc for a better understanding

|

Amir Afghani · Accepted Answer · 2017-01-12 18:44:49Z

0

From your latest comments, it sounds like you simply want to make the Set immutable - which you can do without using any synchronization primitives with :

return Collections.unmodifiableSet(iGraphicSectors);

at the end of this function (See the docs).

As a side note, it's fairly obvious that you don't want to use synchronization, as the Set is constructed locally within the function. It does not have any visibility to other threads executing in your program. If any synchronization is to take place, it's not in this method.

The real question is, will the Set returned by this method change? If so, you can return a synchronized Set from this function with:

return Collections.synchronizedSet(...)

at the end of this function (again, see the docs).

edited Jan 12, 2017 at 18:44

answered Jan 12, 2017 at 18:24

Amir Afghani

38.7k17 gold badges87 silver badges128 bronze badges

Collectives™ on Stack Overflow

Java Concurrency with Sets

2 Answers 2

7 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

7 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related