0

I've seen the use cases of SynchronizedList - they state that upon iterating, even though SynchronizedList is thread-safe, we should use an iterator and a synchronized block like so - 

    synchronized(myList){
    Iterator<Item> iterator = myList.iterator();
    while (iterator.hasNext())
    {
        System.out.println(iterator.next().getMessage());
    }
    }

If I use a ConcurrentHashSet for example (possible in Java 8 using newKeySet() of concurrentHashMap), in a multi-threaded environment, is it still necessary to extract an iterator and use a synchronized block? I tried testing it and it seems unnecessary but I might be missing something.

Thank you!

Guy
  • 145
  • 3
  • 12
  • Although iterator and stream methods will work, don't expect to perform atomic operations or operations over an exact snapshot of the map. For atomic operations or individual entries you'll need the weird `putIfAbsent` and similar methods. To operate on the map as a whole, use a normal map with locks or atomics. – Tom Hawtin - tackline May 12 '19 at 14:43
  • Have you seen this question https://stackoverflow.com/questions/3768554/is-iterating-concurrenthashmap-values-thread-safe – keuleJ May 12 '19 at 15:52

2 Answers2

3

ConcurrentHashMap.newKeySet() returns:

    /**
     * Creates a new {@link Set} backed by a ConcurrentHashMap
     * from the given type to {@code Boolean.TRUE}.
     *
     * @param <K> the element type of the returned set
     * @return the new set
     * @since 1.8
     */
    public static <K> KeySetView<K,Boolean> newKeySet() {
        return new KeySetView<K,Boolean>
            (new ConcurrentHashMap<K,Boolean>(), Boolean.TRUE);
    }

as you can see, it is backed by ConcurrentHashMap. You can use returned instance without any synchronization.

.iterator() method returns a new KeyIterator which is backed by map's Node<K,V>[] table

so, if you iterate in one particular thread that means you will see a snapshot of array of Node and each node in correct state bc Node has volatile links inside, but there is lowest chance you will see new elements added to original map bc the link iterator points is not volatile. In other words, you just iterating over a array without any guarantee if that element still exist in original map atm or some new is added there, but you can see up to date state of each node, bc:

    static class Node<K,V> implements Map.Entry<K,V> {
        final int hash;
        final K key;
        volatile V val;
        volatile Node<K,V> next;

key is final

val is volatile

inkognitto
  • 74
  • 4
  • 1
    Thank you, however I'm not sure it's concurrency promises us thread-safety when iterating (like in a for-loop), if you know something about that, that would be great! – Guy May 12 '19 at 13:51
  • 1
    I just read your edit - so you're saying that using an iterator would be more thread-safe? I didn't quite get that last point - If I use an iterator, I get a copy of the current state of the ConcurrentHashSet, however the set only contains keys and not values, so in fact if someone deleted one of the set's entries, I would be in trouble because I would try to call a function on a removed item. So it seems I might be better off using a regular for-each loop, if I understood you correctly. – Guy May 13 '19 at 08:15
  • 1
    @Guy, Simply speaking, it is thread safe using the iterator in one particular thread. For loop is nice to iterate. You will be iterating through a snapshot of map at some particular moment. I’ve just immersed into sources and tried to argument my answer. – inkognitto May 13 '19 at 12:34
  • Thank you! so what do you think about iterating over the set, in a synchronized block? Like so: synchronized (mySet){ For(Item i: mySet){ doSomething(); } Is the synchronized block required? (I believe it might help prevent some problems, because the synchronized block would allow us to block interference from other threads during the iteration). However since the set is thread-safe, it might be pointless. What do you think? Thank you again! – Guy May 13 '19 at 16:07
  • 1
    @Guy using synchronized (mySet) in one thread and not propagate mySet outside = useless. Moreover, this synchronized will probably be deleted by compiler. – inkognitto May 13 '19 at 21:14
  • What do you mean by "propagate mySet outside"? I didn't quite understand – Guy May 14 '19 at 18:06
  • 1
    @Guy by “propagate mySet outside” I mean to make the link ‘mySet’ reachable for other threads – inkognitto May 15 '19 at 13:48
  • Interesting, so synchronized would make the application run as a single thread for that specific synchronized block that is locked on "mySet", wouldn't improve it's thread-safety and only slow the program down? Is that your point? Thank you again! – Guy May 15 '19 at 14:20
  • 1
    @Guy yep. Synchronization is useless in this case and may slow down your program. – inkognitto May 17 '19 at 07:47
2

For what I know -Each iterator you obtain from a ConcurrentHashMap is designed to be used by a single thread and should not be passed around.

If you try to iterate the Map with more than one thread at the same time it wont work as expected unless each of the threads uses it's own iterator.

The Concurrent of the ConcurrentHashMap with he iterator refer to cases which you are trying to put or remove a value from the map while iterating it - than it will be thread-safe. Though there is no guarantee that the other thread will see the changes unless it is obtaining a new iterator from the map.

I hope the information was usefull !

shani klein
  • 334
  • 1
  • 2
  • 14
  • Interesting, so if I have a method that iterates over that set, and multiple threads might add/remove items from that set, It's better to use the method I described above? that is create a standalone iterator, instead of a "for-each" loop? Also, do you think it makes sense, if I do choose to use a standalone iterator, to also use it in a synchronization block? or will that just be overkill? – Guy May 12 '19 at 13:53
  • mm I'm not sure. Maybe the best way will be trying to do for each loop on ConcurrentHashMap.entrySet())? – shani klein May 12 '19 at 14:08