8

I am trying to understand the SynchronizedMap and I ran the below code. I get the below Output with an exception. According to my understanding the exception is caused when the get() methods are trying to access the syncmap when a thread is still executing a write on the map or Thread is in the Sleep state. Is my understanding correct or am I missing something ?

class MapHelper1 implements Runnable {
Map<String, Integer> map;

public MapHelper1(Map<String, Integer> map) {
    this.map = map;
    new Thread(this, "MapHelper1").start();
}

public void run() {
    map.put("One", 1);
    try {
        System.out.println("MapHelper1 sleeping");
        Thread.sleep(100);
    } catch (Exception e) {
        System.out.println(e);
    }

}
}

class MapHelper2 implements Runnable { 
Map<String, Integer> map;

public MapHelper2(Map<String, Integer> map) {
    this.map = map;
    new Thread(this, "MapHelper3").start();
}

public void run() {
    map.put("two", 1);
    try {
        System.out.println("MapHelper2 sleeping");
        Thread.sleep(100);
    } catch (Exception e) {
        System.out.println(e);
    }

}
}


class MapHelper3 implements Runnable {
Map<String, Integer> map;

public MapHelper3(Map<String, Integer> map) {
    this.map = map;
    new Thread(this, "MapHelper3").start();
}

public void run() {
    map.put("three", 1);
    try {
        System.out.println("MapHelper3 sleeping");
        Thread.sleep(100);
    } catch (Exception e) {
        System.out.println(e);
    }

}
}

public class MainClass {

public static void main(String[] args) {
    Map<String, Integer> hashMap = new HashMap<>();
    Map<String, Integer> syncMap = Collections.synchronizedMap(hashMap);
    MapHelper1 mapHelper1 = new MapHelper1(syncMap);
    MapHelper2 mapHelper2 = new MapHelper2(syncMap);
    MapHelper3 mapHelper3 = new MapHelper3(syncMap);



    for (Map.Entry<String, Integer> e : syncMap.entrySet()) {
        System.out.println(e.getKey() + "=" + e.getValue());
    }

}

}

OUTPUT:

MapHelper1 sleeping
MapHelper2 sleeping
MapHelper3 sleeping


Exception in thread "main" java.util.ConcurrentModificationException
at java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1494)
at java.base/java.util.HashMap$EntryIterator.next(HashMap.java:1527)
at java.base/java.util.HashMap$EntryIterator.next(HashMap.java:1525)
at MainClass.main(MainClass.java:137)
Command exited with non-zero status 1

EDIT : I ran the code again a few times when the output was generated without exception. Why is this behavior?

ghostrider
  • 2,046
  • 3
  • 23
  • 46
  • 1
    This is not a generic map iteration question, but a `Collections.synchronizedMap` problem. It's valid. And to answer: you have to manually synchronize on `syncMap` outside iteration. Look at the [synchronizedMap](https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#synchronizedMap(java.util.Map)) method javadoc – Dariusz Jan 18 '18 at 14:21
  • @Kayaman I understand that the question you linked to is related, but where are entries being removed in this question? Please reconsider opening this question, or maybe find another real duplicate – fps Jan 18 '18 at 14:21
  • @Kayaman I don't think the answer you linked solves my doubt. Can you please remove the duplicate tag. – ghostrider Jan 18 '18 at 14:23
  • @FedericoPeraltaSchaffner while the linked question's title discusses only removal, the post discusses all kinds of modification. The synchronized map has very little to do with this question. Also ghostrider, your understanding is wrong. Calling `get()` doesn't cause an exception because it doesn't modify the collection. Don't iterate and modify a collection unless it explicitly allows it. Whether you do it in a single thread or multiple threads, you'll get a `ConcurrentModificationException`. – Kayaman Jan 18 '18 at 14:24
  • @Kayaman Agreed that the situation is discussed in the answers, however it's not clear that this question is a duplicate, IMO, but as you wish... – fps Jan 18 '18 at 14:26
  • 2
    Fine, I'll reopen this. You better put some good answers in there then. – Kayaman Jan 18 '18 at 14:30
  • @ghostrider The problem is that you can't modify a map (your threads use `put`) while iterating it. And you're iterating the entries in the main thread. This has nothing to do with the map being synchronized, it's all about the iteration. – fps Jan 18 '18 at 14:31
  • @Kayaman Wait, my point was w.r.t. the linked question. I'm sure there must be another question which is a duplicate, let me find it for you – fps Jan 18 '18 at 14:32
  • @FedericoPeraltaSchaffner I tried running the code again. This time it ran fine. Doesn't this contradict your logic about the behavior? – ghostrider Jan 18 '18 at 14:35
  • 1
    @ghostrider No, because there's a race condition between the 3 puts getting the iterator over the entry set. If all 3 puts happen before getting the iterator, then it will run OK, but you can't trust that this will always happen this way – fps Jan 18 '18 at 14:37
  • @Kayaman Here's the duplicate, though it's on a list... [https://stackoverflow.com/questions/1496180/concurrent-modification-exception](https://stackoverflow.com/questions/1496180/concurrent-modification-exception) – fps Jan 18 '18 at 14:41
  • @FedericoPeraltaSchaffner Thanks, Yes I get it now.. so what purpose does the synchronizedmap serve in terms of reading and writing the same map. In this question do the threads acquire the lock for the entire map and that is released only when the Thread.sleep() completes its execution? And during this entire period does the read thread(if any) has to wait for the final edits? – ghostrider Jan 18 '18 at 14:45
  • 1
    @ghostrider yes, though the read thread might kick-in between one write operation and another, i.e. there's no guaranteed order. The sync map is useful for read/write "atomic" operations. Iterating through the entire map is not an "atomic" read operation, but a "bulk" read operation (to name it in some way). The underlying iterator fetches "atomically" one entry at a time. In this sense, the map is synchronized, but not for the whole traversal. – fps Jan 18 '18 at 15:21

2 Answers2

8

You dont synchronize access when iterating. Use:

synchronized(syncMap) {
  for (Map.Entry<String, Integer> e : syncMap.entrySet()) {
    System.out.println(e.getKey() + "=" + e.getValue());
  }
}

It's even in the synchronizedMap() method javadoc

It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views:

  Map m = Collections.synchronizedMap(new HashMap());
      ...
  Set s = m.keySet();  // Needn't be in synchronized block
      ...
  synchronized (m) {  // Synchronizing on m, not s!
      Iterator i = s.iterator(); // Must be in synchronized block
      while (i.hasNext())
          foo(i.next());
  }
Dariusz
  • 21,561
  • 9
  • 74
  • 114
  • I ran the code again a few times when the output was generated without exception. Why is this behavior? – ghostrider Jan 18 '18 at 14:38
  • Synchronization errors are generally not reproducible. You get different results from each run. Nobody knows of the bug will surface again in the next run or not. @ghostrider – Ole V.V. Jan 18 '18 at 14:42
  • @OleV.V. Thanks, Yes I get it now.. so what purpose does the synchronizedmap serve in terms of reading and writing the same map. In this question do the threads acquire the lock for the entire map and that is released only when the Thread.sleep() completes its execution? And during this entire period does the read thread(if any) has to wait for the final edits? – ghostrider Jan 18 '18 at 14:48
2

I just read @Dariusz answer, I think it's totally correct.

To answer specifically why you saw random behavior, it's completely timing. That is, if the map had been populated by all three threads before iteration in Main thread, then everything is fine. But if one thread try to populate while iteration is already underway in Main thread, then you get the exception.

Btw, I know this is for demo purpose. But in real code, better not to start the thread in constructor.

glithedev
  • 56
  • 3
  • 1
    btw, I put in answer rather than comment because I don't yet have reputation to comment. – glithedev Jan 18 '18 at 14:52
  • Thanks, Yes I get it now.. so what purpose does the synchronizedmap serve in terms of reading and writing the same map. In this question do the threads acquire the lock for the entire map and that is released only when the Thread.sleep() completes its execution? And during this entire period does the read thread(if any) has to wait for the final edits? – ghostrider Jan 18 '18 at 14:54
  • 1
    1 the lock is release right after those 'put' calls return – glithedev Jan 18 '18 at 15:16
  • 1
    2 all threads need to lock on the same lock, which is the syncMap itself in this case; 3 syncMap.entrySet() call actually acquires and then release the same lock (syncMap itself), but the underlying map entry data is shared still, therefore the ConcurrentModification Exception. As matter of fact, one can get such exception within single thread, because the root cause is iteration checks against a snapshot taken at the beginning of iteration. 4, ConcurrentMap likely is not what want here, but another tool when you need it. – glithedev Jan 18 '18 at 15:25
  • 1
    additional note that should help. Even with syncing on syncMap in Main thread, the program can still show random results depending on timing. The ConcurrentModificationException will be gone for sure, but it can print 0, 1, 2 etc entries etc. If the Main thread need to collect data after all worker done writing, then you need thread synchronizer. data access lock alone won't do it in this case. – glithedev Jan 18 '18 at 15:48
  • awesome explanation..just one doubt here dont the Thread.sleep() acquire any lock here? Or does it come into picture when that block is synchronized? – ghostrider Jan 18 '18 at 17:00
  • the sleep() call has nothing to do with lock acquisition or release. timing wise, maybe think of it as a busy loop taking the same amount of time (as passed into sleep call.), but sleep() is more efficient in terms of system resource. – glithedev Jan 18 '18 at 18:42