Pre-Java 5 threads

I would like to revise the fundamentals of Java threads on before-Java 5, so that I can understand improvements in Java 5 and beyond.

I started off with a custom collection and need help on:

• Things I am doing wrong / grossly overlooking

• How to make it better

• Suggest alternate implementation, if any

• Which is the correct place to have the kill variable? Is it inside the thread

or in the collection like I did?

```
public class ThreadCreation {

public static void main(String[] args) {
MyCollection coll = new MyCollection();
coll.kill = false;

RemoveCollClass t2 = new RemoveCollClass();
t2.setName("Thread2");
t2.coll = coll;
t2.start();

AddCollClass t1 = new AddCollClass();
t1.setName("Thread1");
t1.coll = coll;
t1.start();

RemoveCollClass t3 = new RemoveCollClass();
t3.setName("Thread3");
t3.coll = coll;
t3.start();

try {
Thread.sleep(10000);
} catch (InterruptedException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}

coll.kill = true;
}

static class AddCollClass extends Thread {

volatile MyCollection coll;
int count = 0;

@Override
public void run() {

while (!coll.kill) {
System.out.println(Thread.currentThread().getName()
+ " --> AddCollClass Attempt -->" + count);
coll.add();
count++;
try {
Thread.sleep(2000);
} catch (InterruptedException e) {
// TODO Auto-generated catch block
e.printStackTrace();
}
}

}
}

static class RemoveCollClass extends Thread {

volatile MyCollection coll;
int count = 0;

@Override
public void run() {
// System.out.prin

Regardless of which version of Java you are using, there are a few things you have 'wrong' when it comes to best-practice threading in Java.

The Big Picture

• avoid volatile unless you have no choice (especially pre-Java5 - In Java5 and later the volatile keyword introduces more predictable behaviour).

• in this program, you have a choice, and you don't need volatile.

• avoid extending thread. Instead, extend Runnable and use the runnable as a Thread constructor.

• never synchronize on the methods unless you really, really have to, and you do not have to.

Synchronization

By synchronizing on the methods, it means anyone can mess up your program. Consider a person who decides to use your class as some construct in their program, and they do:

private final MyCollection mycoll = new MyCollection();

public void methodABC() {
    synchronized(mycoll) {
        // do a bunch of stuff
    }
}

Suddenly your synchronization is screwed.... and deadlocks happen. See this Stack Overflow discussion.

Since, your MyCollection class has the collection Stack instance, use that as your monitor lock..... (and make it a private-final as well:

static class MyCollection {
    private final Stack container = new Stack();

    int maxSize = 5;
    volatile boolean kill = false;

    public boolean isFull() {
        synchronized (container) {
            return container.size() >= maxSize;
        }
    }

Also, you should use notifyAll() and not just notify(). This is especially true when you use method/instance synchronization, because anyone outside your class will potentially get the notify, if they are the ones blocking your class.... You may end up notifying some other thread that is not part of your class.... actually, there is a real bug there:

Bug: Incorrect notification.

If two threads remove an item from a full collection, and two other threads are waiting for space to put things in .... then both the removing threads may notify the same adding thread, and the second adding thread may still be waiting even through there is space....

use notifyAll();

Volatile

Volatile before Java5 was somewhat broken. Don;t use it.

None of your methods block while holding the lock (they all 'wait' which releases the lock...). As a result, your lock-holds are really short.

There is no reason to separate the kill as a volatile then. Simply have a kill and isKilled method.

When you can understand this article, you can use volatile

Runnable

All your 'Thread' classes should instead be Runnable.

They should have private final variables, and not have volatile at all. For example, this class:

static class RemoveCollClass extends Thread {

    volatile MyCollection coll;
    int count = 0;

    @Override
    public void run() {
        // System.out.println("ThreadClass is running ");

        while (!coll.kill) {
            System.out.println(Thread.currentThread().getName()
                    + " -->RemoveCollClass Attempt -->" + count);
            coll.remove();
            count++;

        }

    }
}

Should look like:

static class RemoveCollClass implements Runnable {

    private final MyCollection coll;
    int count = 0;

    public RemoveCollClass(MyCollection coll) {
        super();
        this.coll = coll;
    }

    public void run() {
        // System.out.println("ThreadClass is running ");

        while (!coll.isKilled()) {
            System.out.println(Thread.currentThread().getName()
                    + " -->RemoveCollClass Attempt -->" + count);
            coll.remove();
            count++;

        }

    }
}

Then, use the Runnable as a Thread constructor:

RemoveCollClass rcc2 = new RemoveCollClass(coll);
    Thread t2 = new Thread(rcc2, "Thread2");
    t2.setDaemon(true);
    t2.start();

The Small Things:

-
Simplify your logic:

public synchronized boolean isEmpty() {
        if (container.size() <= 0)
            return true;
        else
            return false;
    }

This code can be simply:

public synchronized boolean isFull() {
    return container.size() >= maxSize;
}

public synchronized boolean isEmpty() {
    return container.empty();
}

• //TODO in catch block ... why?

} catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                    notify();
                }

• Don't use i-liner if/else blocks. They lead to bugs:

if (container.size() <= 0)
            return true;

StackExchange Code Review Q#47030, answer score: 4