java multithreading synchronized block not working

Question

I am not able to get why I am getting exception with both : class level lock as well as with object level lock in below code :

It seems object level locking should work here as we are changing and accessing hm(Object) value using different threads, but still we are getting exception(java.util.ConcurrentModificationException). I tried with all three locking commented in the code. I know using Hashtable or ConcurrentHashMap we can resolve this problem, but I want to know the concept which is missing using HashMap.

import java.util.HashMap;
class A{
    StringBuilder str = new StringBuilder("ABCD");
    StringBuilder exception = new StringBuilder("");
    HashMap hm = new HashMap();
    public void change() {
        //synchronized(A.class) {
        //synchronized (this){
        synchronized (hm){
            (this.str).append(Thread.currentThread().getName().toString());
            System.out.println(Thread.currentThread().getName()+"::::::"+str);
            hm.put(Thread.currentThread(), Thread.currentThread());
        }
    }
    public void impact() {
        //synchronized(A.class) {
        //synchronized(this) {
        synchronized(hm) {
            System.out.println(Thread.currentThread()+"...Inside impact :::"+hm.get(Thread.currentThread()));
        }
    }
    public void print() {
        System.out.println("Inside print :::"+str);
        System.out.println("Inside print :::exception--"+exception);
    }
}
class B extends Thread{
    A a;
    B(A a){
        this.a=a;
    }
    public void run() {
        try {
            System.out.println("Inside B run::"+a.hm);
            a.change();
            a.impact();
        }
        catch(Exception e){
            StringWriter sw = new StringWriter();
            e.printStackTrace(new PrintWriter(sw));
            System.out.println(sw.toString());
            a.exception.append(sw.toString());
            try {
                sw.close();
            } catch (IOException e1) {
                e1.printStackTrace();
            }
        }
    }
}

class C extends Thread{
    A a;
    C(A a){
        this.a=a;
    }
    public void run() {
    try {
        System.out.println("Inside C run::"+a.hm);
        a.change();
        a.impact();
    }
    catch(Exception e){
        StringWriter sw = new StringWriter();
        e.printStackTrace(new PrintWriter(sw));
        System.out.println(sw.toString());
        a.exception.append(sw.toString());
        try {
            sw.close();
        } catch (IOException e1) {
            e1.printStackTrace();
        }
    }
    }
}
public class multiTest {
    public static void main(String[] args) {
    // TODO Auto-generated method stub
    A a = new A();
    for(int i=0;i<=100;i++) {
        B b = new B(a);
        C c = new C(a);
        b.start();
        c.start();
    }
    try {
        Thread.currentThread().sleep(5000);
    }catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
        a.print();  
    }
}

*I am getting exception*: what is the complete stack trace of the exception? Also, your print() method accesses str and exception without any synchronization. And your B thread prints hm outside of any synchronization block. And it uses exception the same way. Same for C. 1. Make fields private. 2. stop using raw types. 3. make sure **every** access to a shared state is synchronized, on the same lock. — JB Nizet, Jul 17 '18 at 06:35
[I downvoted because your question is missing exception details](http://idownvotedbecau.se/noexceptiondetails/) — Andreas, Jul 17 '18 at 06:37
The stacktrace would likely show that problem occurs during `HashMap.toString()` call, from one of the two `println` of `hm`, since those would iterate the map without being protected by `synchronized`. — Andreas, Jul 17 '18 at 06:38
@JBNizet Time to go home I think. Let's pretend I was never here. — Scary Wombat, Jul 17 '18 at 06:47
@JBNizet I added exceptions in the exception list :: java.util.ConcurrentModificationException which will be getting added each time we will get exception. — RVP, Jul 17 '18 at 07:19
@JBNizet, **print()** method does not need any synchronization as we are accessing it from main thread only. #Correcting previous comment **exception** String — RVP, Jul 17 '18 at 07:31
@RVP yes, it does. The main thread has nothing special. It's a thread, like any other thread, and if it accesses shgared state concurrently, then it needs synchronization. Besides, nothing prevents any other thread to use print(), so if you want your class to be thread-safe, then its print() method should be, too. — JB Nizet, Jul 17 '18 at 07:34
@Andreas **stacktrace** :: Thread[Thread-27,5,main]...Inside impact :::Thread[Thread-27,5,main] Inside print :::exception--java.util.ConcurrentModificationExceptionjava.util.ConcurrentModificationExceptionjava.util.ConcurrentModificationExceptionjava.util.ConcurrentModificationException — RVP, Jul 17 '18 at 07:35
@RVP That is not a stacktrace. Try actually *printing* the stacktrace using `e.printStackTrace()` — Andreas, Jul 17 '18 at 15:32

score 1 · Accepted Answer · answered Jul 17 '18 at 07:01

1

The problem is on this line:

System.out.println("Inside B run::"+a.hm);

There is sneaky implicit invocation of a.hm.toString() here, and that does sneaky iteration of the map's entries; but you aren't synchronizing on anything, so you don't have exclusive access to the hashmap.

Put it in a synchronized block:

synchronized (a.hm) {
  System.out.println("Inside B run::"+a.hm);
}

(And make hm final; and don't use raw types).

answered Jul 17 '18 at 07:01

Andy Turner

137,514
11
162
243

Thanks @Andy_Turner. It seems problem was only because of mentioned println. Thanks for suggestions. – RVP Jul 17 '18 at 07:22
@RVP Sounds like you found this answer useful, so you should up-vote it, i.e. click the up-arrow to the left of the answer. Since it also solved your issue, you should click the checkmark too, to accept the answer, so others can see that you question has been answered to your satisfaction. – Andreas Jul 17 '18 at 15:35
@RVP Really? I see no upvote other than mine, and the answer hasn't been accepted, so you've *not* done it already. Still. – Andreas Jul 18 '18 at 02:36
@Andreas, I have done, Can you see is it up from your side? as I am unable to do it twice, tried again after your comment. It was saying Thanks for the feedback..... and now also same. – RVP Jul 19 '18 at 04:20

java multithreading synchronized block not working

1 Answers1