Getting IllegalMonitorStateException while printing arraylist using threads

I am trying to print out the content of arraylist using 2 threads, my main goal is to make threads read arraylist in a synchronized way and print its content. Eventhough I use synchronized block, I still am getting IllegalMonitorStateException. I know this is a basic question but I can not get it working, pardon me.

import java.util.*;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class Q1 {
public static Q1 yeni;

public static void main(String[] args) {
    // TODO Auto-generated method stub
    yeni = new Q1();
}

Q1() {

    List<String> list = Collections.synchronizedList(new ArrayList<String>());

    list.add("a1");
    list.add("b1");
    list.add("c1");
    list.add("d1");
    list.add("e1");
    list.add("f1");
    ExecutorService executorService = Executors.newFixedThreadPool(10);

    synchronized (list) {
        myThread thread1 = new myThread(list);
        myThread thread2 = new myThread(list);
        thread1.start();
        thread2.start();

    }
    }

    }

And here is myThread class

import java.util.*;

public class myThread extends Thread {
List<String> liste;

public myThread(List<String> liste) {
    this.liste = liste;
}

@Override
public void run() {
    try {
        synchronized (Q1.yeni) {
            System.out.println("Thread number " + this.getName() + " started running.");
            for (int i = 0; i < liste.size(); i++) {
                System.out.println(liste.get(i));
                this.wait(3000);
            }
        }
    } catch (InterruptedException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    }

    }

Answer

The reason for the IllegalMonitorStateException is that you are calling wait on an object (this) without holding the monitor for that object. You must either wrap this call with a synchronized (this) block, or call wait on Q1.yeni, which this code already has synchronized.

However, it looks like the use of wait might be mistaken. This method is used to wait on a condition, which is signaled with a call to notify or notifyAll on the same object. Since there is no apparent condition in this code, and no usages of notify or notifyAll, I suspect that what you really want to call is this.sleep(3000), which pauses the thread for three seconds, then resumes it after that duration elapses.

The sleep method does not require ownership of any monitors, and does not release ownership of held monitors, so another thread would not be able to enter the synchronized (Q1.yeni) block while one is currently sleeping. This implies that the first thread to enter that block will run to completion, iterating through the entire list, before the second thread has a chance to begin. It’s not totally clear if that’s what is intended here.

See the documentation for Object.wait and Thread.sleep for more usage information.

A second problem is that Q1.yeni is accessed by these threads before it is necessarily initialized, because the threads are started in the Q1 constructor, and the statement yeni = new Q1(); only assigns yeni after the constructor completes. In this case, it might be better for the threads to use synchronized (liste) instead.

Other than that, having synchronized (list) in the Q1 constructor does not accomplish much, since the main thread does not access or manipulate the contents of list in that section. The only practical effect is that the threads it starts will block when they reach the first call to liste.size() until the main thread exits the synchronized (list) (immediately after starting the two threads). This has the potential to slightly slow down the first thread that runs, but has no effect on the thread-safety or correctness of the program.

I would also recommend reviewing “How to Handle InterruptedException in Java”. In this case, I would recommend restoring the interruption status in the exception handler.

Put together, here is a revised example of this code (including other minor changes to remove unused code and boilerplate comments, improve formatting, and ensure consistency with Java naming conventions):

Q1.java:

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class Q1 {
    private static Q1 yeni;

    public static void main(String[] args) {
        yeni = new Q1();
    }

    Q1() {
        List<String> list = Collections.synchronizedList(new ArrayList<>());
        list.add("a1");
        list.add("b1");
        list.add("c1");
        list.add("d1");
        list.add("e1");
        list.add("f1");

        MyThread thread1 = new MyThread(list);
        MyThread thread2 = new MyThread(list);
        thread1.start();
        thread2.start();
    }
}

MyThread.java:

import java.util.*;

public class MyThread extends Thread {
    private final List<String> liste;

    public MyThread(List<String> liste) {
        this.liste = liste;
    }

    @Override
    public void run() {
        try {
            synchronized (liste) {
                System.out.println("Thread number " + this.getName() + " started running.");
                for (int i = 0; i < liste.size(); i++) {
                    System.out.println(liste.get(i));
                    sleep(3000);
                }
            }
        } catch (InterruptedException e) {
            e.printStackTrace();
            interrupt();
        }
    }
}

Output:

Thread number Thread-0 started running.
a1
b1
c1
d1
e1
f1
Thread number Thread-1 started running.
a1
b1
c1
d1
e1
f1