Java : synchronizing on custom collection class

Question

I want to have shared collection class which is filled by producer thread and output is shown by consumer thread. It's sometimes working with 0 element of the collection class but never goes further. In Eclipse I observer "DestroyJVM" thread after application freezes. There is artificial latency on the producer to simulate "slow" producer. I don't have an idea why application is not working in sequence, like "Producer acquires lock on collection class, adds Integer, consumer waits, producer releases lock, consumer acquires lock, consumer prints, consumer releases lock, producer acquires..." and so on. Can anyone point out where is the mistake?

Here is my code:

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

import static java.lang.System.out;

public class SyncOwnCollMain {

    public static void main(String[] args) {
        SharedIntegers ints = new SharedIntegers();
        Producer producer = new Producer();
        Consumer consumer = new Consumer();
        producer.setInts(ints);
        consumer.setInts(ints);
        Thread producerThread = new Thread(producer);
        producerThread.setName("ProducerThread");
        Thread consumerThread = new Thread(consumer);
        consumerThread.setName("ConsumerThread");

        producerThread.start();
        consumerThread.start();
    }

}

class SharedIntegers {
    private final List<Integer> ints = new ArrayList<Integer>();
    private final int max = 100;

    public synchronized void addAtPosition(int i, Integer integer) {
            ints.add(i, integer);
    }
    public synchronized Integer getAtPosition(int i) {
            return ints.get(i);
    }
    public synchronized Integer removeAtPosition(int i) {
            return ints.remove(i);
    }
    public synchronized Integer getSize() {
            return ints.size();
    }
    public synchronized boolean isFinished() {
            return max < ints.size();
    }
}

class Producer implements Runnable {
    private SharedIntegers ints;
    private int timeout = 100;

    public SharedIntegers getInts() {
            return ints;
    }

    public void setInts(SharedIntegers ints) {
            this.ints = ints;
    }

    @Override
    public void run() {
        out.println("Started ProducerThread");
        if (getInts() != null) {
            int i = 0;
            Integer integer = null;
            while (!getInts().isFinished()) {
                synchronized (getInts()) {
                    integer = i * 3;
                    getInts().addAtPosition(i, integer);
                    out.print("Producer added new integer = " + integer + " at " + i + " position");
                    out.println(". Will sleep now for " + timeout + " ms");
                    try {
                        Thread.sleep(timeout);
                        getInts().wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                    i++;
                }
            }
        }
    }
}

class Consumer implements Runnable {
    private SharedIntegers ints;

    public SharedIntegers getInts() {
            return ints;
    }

    public void setInts(SharedIntegers ints) {
            this.ints = ints;
    }

    @Override
    public void run() {
        out.println("Started ConsumerThread");
        if (getInts() != null && getInts().getSize() > 0) {
            int i = 0;
            while (!getInts().isFinished()) {
                synchronized (getInts()) {
                    showAtPosition(i, getInts());
                    i++;
                    try {
                        getInts().wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                    Thread.yield();
                }
            }
        } else {
            Thread.yield();
        }
    }

    private void showAtPosition(int position, SharedIntegers ints) {
            out.println("sharedInts[" + position + "] -> " + ints.getAtPosition(position));
    }
}

EDITED: I managed to rewrite code so that it will work in the desired manner, however, producerThread and consumerThread don't exit gracefully. Any ideas why?

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

import static java.lang.System.out;

public class SyncOwnCollMain {

    public static void main(String[] args) {
        out.println("Main application started");
        SharedIntegers ints = new SharedIntegers();
        Producer producer = new Producer();
        Consumer consumer = new Consumer();
        producer.setInts(ints);
        consumer.setInts(ints);
        Thread producerThread = new Thread(producer);
        producerThread.setName("ProducerThread");
        Thread consumerThread = new Thread(consumer);
        consumerThread.setName("ConsumerThread");

        consumerThread.start();
        try {
            Thread.sleep(1000); // simulate that consumerThread is "anxious" to start
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        producerThread.start();
        try {
            consumerThread.join(); //let consumerThread finish before main()
            producerThread.join(); //let producerThread finish before main()
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        out.println("Main application finished");
    }

}

class SharedIntegers {
    private final List<Integer> ints = new ArrayList<Integer>();
    private final int max = 5;

    public synchronized void addAtPosition(int i, Integer integer) {
            ints.add(i, integer);
    }
    public synchronized Integer getAtPosition(int i) {
            return ints.get(i);
    }
    public synchronized Integer removeAtPosition(int i) {
            return ints.remove(i);
    }
    public synchronized Integer getSize() {
            return ints.size();
    }
    public synchronized boolean isFinished() {
            return max <= ints.size();
    }
    public synchronized boolean overflow(int i) {
            return i >= max;
    }
}

class Producer implements Runnable {
    private SharedIntegers ints;
    private final int timeout = 500;

    public SharedIntegers getInts() {
            return ints;
    }

    public void setInts(SharedIntegers ints) {
            this.ints = ints;
    }

    @Override
    public void run() {
        out.println("Started ProducerThread");
        if (getInts() != null) {
            int i = 0;
            Integer integer = null;
            synchronized (getInts()) {
                while (!getInts().isFinished()) {
                    integer = i * 3;
                    getInts().addAtPosition(i, integer);
                    out.print("Producer added new integer = " + integer + " at " + i + " position");
                    out.println(". Will sleep now for " + timeout + " ms");
                    try {
                        getInts().notify();
                        getInts().wait();
                        Thread.sleep(timeout); // simulate "slow" producer
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                    i++;
                }
                try {
                    getInts().wait();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }
        }
        out.println("Finished ProducerThread");
    }
}

class Consumer implements Runnable {
    private SharedIntegers ints;

    public SharedIntegers getInts() {
            return ints;
    }

    public void setInts(SharedIntegers ints) {
            this.ints = ints;
    }

    @Override
    public void run() {
        out.println("Started ConsumerThread");
        if (getInts() != null) {
            synchronized (getInts()) {
                int i = 0;
                while (!getInts().overflow(i)) {
                    if (getInts().getSize() > 0) {
                        showAtPosition(i, getInts());
                        i++;
                    }
                    try {
                        getInts().notify();
                        getInts().wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
        out.println("Finished ConsumerThread");
    }

    private void showAtPosition(int position, SharedIntegers ints) {
            out.println("sharedInts[" + position + "] -> " + ints.getAtPosition(position));
    }
}

EDIT 2: solution found : needed to notify consumerThread from producerThread that getInts() lock can be re-acquired. The working code with my comments looks like this (added some data modification by consumerThread):

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

import static java.lang.System.out;

public class SyncOwnCollMain {

    public static void main(String[] args) {
        out.println("Main application started");
        SharedIntegers ints = new SharedIntegers();
        Producer producer = new Producer();
        Consumer consumer = new Consumer();
        producer.setInts(ints);
        consumer.setInts(ints);
        Thread producerThread = new Thread(producer);
        producerThread.setName("ProducerThread");
        Thread consumerThread = new Thread(consumer);
        consumerThread.setName("ConsumerThread");

        consumerThread.start();
        try {
            Thread.sleep(1000); // simulate that consumerThread is "anxious" to start
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        producerThread.start();
        try {
            consumerThread.join(); //let consumerThread finish before main()
            producerThread.join(); //let producerThread finish before main()
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        out.println("Main application finished"); // here, main() thread has result produced by producerThread and consumerThread
    }

}

class SharedIntegers {
    private final List<Integer> ints = new ArrayList<Integer>();
    private final int max = 5;

    public synchronized void addAtPosition(int i, Integer integer) {
            ints.add(i, integer);
    }
    public synchronized Integer getAtPosition(int i) {
            return ints.get(i);
    }
    public synchronized Integer removeAtPosition(int i) {
            return ints.remove(i);
    }
    public synchronized Integer getSize() {
            return ints.size();
    }
    public synchronized boolean isFinished() {
            return max <= ints.size();
    }
    public synchronized boolean overflow(int i) {
            return i >= max;
    }
}

class Producer implements Runnable {
    private SharedIntegers ints;
    private final int timeout = 500;

    public SharedIntegers getInts() {
            return ints;
    }

    public void setInts(SharedIntegers ints) {
            this.ints = ints;
    }

    @Override
    public void run() {
        out.println("Started ProducerThread");
        if (getInts() != null) {
            int i = 0;
            Integer integer = null;
            synchronized (getInts()) {
                while (!getInts().isFinished()) {
                    integer = i * 3;
                    getInts().addAtPosition(i, integer);
                    out.print("Producer added new integer = " + integer + " at " + i + " position");
                    out.println(". Will sleep now for " + timeout + " ms");
                    try {
                        getInts().notifyAll(); // notify all threads (in this case - consumer thread) that getInts() will be available for other threads to sync and other threads are legitimate to compete for locking getInts()
                        getInts().wait(); // release lock for getInts()
                        Thread.sleep(timeout); // simulate "slow" producer
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                    i++;
                }
                out.println("Finished ProducerThread while() loop");
                getInts().notifyAll(); // after job is done, need to notify consumer thread that it can compete to obtain getInts() lock
            }
        }   
    }
}

class Consumer implements Runnable {
    private SharedIntegers ints;

    public SharedIntegers getInts() {
            return ints;
    }

    public void setInts(SharedIntegers ints) {
            this.ints = ints;
    }

    @Override
    public void run() {
        out.println("Started ConsumerThread");
        if (getInts() != null) {
            int i = 0;
            synchronized (getInts()) {
                while (!getInts().overflow(i)) {
                    if (getInts().getSize() > 0) {
                        out.println(showAtPosition(i, getInts()));
                        increaseAtPosition(i, getInts());
                        out.println("After consumer increase : " + showAtPosition(i, getInts()));
                        i++;
                    }
                    try {
                        getInts().notifyAll(); // notify all threads that other threads are legitimate to compete for getInts() lock
                        getInts().wait(); // release getInts() lock, wait for allowance notification
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
                out.println("Finished ConsumerThread while() loop");
            }
        }
    }

    private String showAtPosition(int position, SharedIntegers ints) {
            return "sharedInts[" + position + "] -> " + ints.getAtPosition(position);
    }

    private void increaseAtPosition(int position, SharedIntegers ints) {
        Integer increased = ints.getAtPosition(position)+1;
        ints.removeAtPosition(position);
        ints.addAtPosition(position, increased);
    }
}

Why are you writing this with calls to `wait()` and `notify()`? The abstractions in java.util.concurrent (Java Executor Framework) make exactly this sort of thing easier to write and maintain. — scottb, Jul 29 '15 at 13:32
This is my application to get familiar with threads, how threads interact, how to force one thread wait for another, how 2 threads can access shared resource and how force threads get results I want since concurrent programming is not like sequential programming. — t3rmin41, Jul 29 '15 at 13:44

score 0 · Answer 1 · answered Jul 28 '15 at 15:06

0

Your call to getInts().wait(); causes each Thread to wait forever, since you never call notify() , therefore your application freezes. See the Javadoc for java.lang.Object.wait() and java.lang.Object.notify()

answered Jul 28 '15 at 15:06

hinneLinks

3,673
26
40

*"getInts().wait(); causes each Thread to wait forever"* how exactly? getInts().wait() releases the lock on getInts(), call on getInts().notify() doesn't release the lock on getInts() – t3rmin41 Jul 28 '15 at 15:30
If you say that I should call getInts().notify() where should I place it? Inside sync() {} block as last line (I guess both for Producer and Consumer)? Just outside sync(){} block? Both variants don't have any effect, I get same result. – t3rmin41 Jul 28 '15 at 15:50
I would say you don't need wait and notify at all, you use wait to tell a thread to sleep infinitely, and notify to wake him up. Why do your threads need to do that? If youcare about to release the Locks on getInts() - its automattically released when you exit the syncronized-block or the syncronized methods, btw, it woudl be propably easier to just use syncronized-blocks on `ints` in every method of SharedIntegers and remove the syncronized methods and other blocks. – hinneLinks Jul 29 '15 at 06:14
I don't intent to use getInts().wait() to sleep inifinitely. By the way, getInts().notify() did really help (thanks for your insight), I inserted just before each getInts().wait() and slightly edited the code (see original question under mark "EDITED"), now it works in the manner I wanted. However, producerThread and consumerThread don't get terminated. Any ideas how to "gracefully" exit both threads? – t3rmin41 Jul 29 '15 at 13:15
I guess the threads are still sleeping. In your JDK/bin-Folder you have a tool called "jvisualvm". With that you can monitor your application - open the "Threads" tab, there you'll see what your Threads are doing. With the Button "Thread Dump" you'll see in which line of code they are. And again, i don't see any use of the wait/notify, if you don't want the threads to run parallel just use one thread. – hinneLinks Jul 30 '15 at 05:34
The tool shows that producerThread is always sleeping and consumerThread is always waiting. But how then those threads fill in SharedIntegers object if one is sleeping and the other is waiting? The use of "wait/notify" would be to allow producer run for small amount of time [put Integer into list], then wait for consumer, consumer then run for small amount of time [print result, increase], wait for consumer and the sequence repeats. If you can tell how to achieve such result, I would be glad to hear it. – t3rmin41 Jul 30 '15 at 13:52

score 0 · Answer 2 · answered Jul 28 '15 at 16:41

0

Inside the Producer, change

getInts().wait()

to

getInts().notify()

answered Jul 28 '15 at 16:41

cahen

15,807
13
47
78

Well, not really. I want Producer and Consumer to execute in sequence *Consumer->Producer->Consumer->Producer* and so on so that output would be: `Producer added new integer = 0 at 0 position. sharedInts[0] -> 0 Producer added new integer = 3 at 1 position. sharedInts[1] -> 3` Now it's like `Producer added new integer = 0 at 0 position. Producer added new integer = 3 at 1 position. sharedInts[0] -> 0 Producer added new integer = 6 at 2 position. Producer added new integer = 9 at 3 position. sharedInts[1] -> 3` – t3rmin41 Jul 28 '15 at 16:50
I just pointed you to the immediate problem since the application didn't even ran. If you want the Producer to be notified by the Consumer that it consumed and vice-versa, than use the `Observer` pattern because it's not a common Consumer/Producer concurrency problem anymore. – cahen Jul 28 '15 at 17:01
*since the application didn't even ran.* I guess you mean *didn't finish*. OK, I'll search for Observer pattern. – t3rmin41 Jul 28 '15 at 17:04
Though I still want to know how can I fix my code to make it work in desired way – t3rmin41 Jul 28 '15 at 17:43

Java : synchronizing on custom collection class

2 Answers2