java - question about thread abortion and deadlock - volatile keyword

Question

Hello all,

I am having some troubles to understand how I have to stop a running thread. I'll try to explain it by example. Assume the following class:

public class MyThread extends Thread {
    protected volatile boolean running = true;

    public void run() {
        while (running) {
            synchronized (someObject) {
                while (someObject.someCondition() == false && running) {
                    try {
                        someObject.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
                // do something useful with someObject
            }
        }
    }

    public void halt() {
        running = false;
        interrupt();
    }
}

Assume the thread is running and the following statement is evaluated to true:

while (someObject.someCondition() == false && running)

Then, another thread calls MyThread.halt(). Eventhough this function sets 'running' to false (which is a volatile boolean) and interrupts the thread, the following statement is still executed:

someObject.wait();

We have a deadlock. The thread will never be halted.

Then I came up with this, but I am not sure if it is correct:

public class MyThread extends Thread {
    protected volatile boolean running = true;

    public void run() {
        while (running) {
            synchronized (someObject) {
                while (someObject.someCondition() == false && running) {
                    try {
                        someObject.wait();
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
                // do something useful with someObject
            }
        }
    }

    public void halt() {
        running = false;
        synchronized(someObject) {
            interrupt();
        }
    }
}

Is this correct? Is this the most common way to do this?

This seems like an obvious question, but I fail to come up with a solution. Thanks a lot for your help.

Answer 1

The interrupt() call will set a flag in the interrupted thread, someObject.wait() will always check this flag, so your first class should work. AFAIC the first is the common way, your error has to be somewhere else.

Answer 2

I tried to simulate the first version, and indeed, the interrupted flag is remembered. I did not know that. Here is my code how I simulated it:

public class Test {
    protected static class MyThread extends Thread {
        protected Object someObject = new Object();

        public void run() {
            for (int i = 0; i < Integer.MAX_VALUE; ++i) {
                /* this takes some time */
            }
            try {
                synchronized (someObject) {
                    someObject.wait();
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            System.out.println("WE COME HERE AFTER INTERRUPTED EXCEPTION");
            try {
                synchronized (someObject) {
                    someObject.wait();
                }
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            System.out.println("HOWEVER, WE NEVER COME HERE, THE INTERRUPTED FLAG SEEMS TO BE RESETTED");
        }

        public void halt() {
            interrupt();
        }
    }

    public static void main(String[] a) throws InterruptedException {
        MyThread t = new MyThread();
        t.start();
        t.halt();
    }
}

This makes me wondering. Why is

public void halt() {
    interrupt();
}

prefered above

public void halt() {
    synchronized(someObject) {
        someObject.notifyAll();
    }
}

In both versions, the while(...) will be evaluated again?

Answer 3

In the catch block for InterruptedException you should include:

if (!running)
    break main;

and label your while (running) loop like main: while (running)

There is no need to synchronize on someObject in order to call interrupt.

Also, I suggest that you rename your running variable because it is very confusing. I suggest shouldContinue. Thus:

public class MyThread extends Thread {
    private volatile boolean shouldContinue = true;

    public void run() {
        main: while (shouldContinue) {
            synchronized (someObject) {
                while (someObject.someCondition() == false && shouldContinue) {
                    try {
                        someObject.wait();
                    } catch (InterruptedException e) {
                        if (!shouldContinue) {
                            break main;
                        }
                    }
                }
                // do something useful with someObject
            }
        }
    }

    public void halt() {
        shouldContinue = false;
        interrupt();
    }
}

Answer 4

Alternatively.... you can use the overload wait method that takes as input the max milliseconds to wait. Now in your halt method you may just set running=false and rest guaranteed that after the specified milliseconds the wait call will end and while condition will be evaluated again.

In the modified code below, the wait will block no longer than 1 second, and then the while loop condition will be checked again. This time as running is set to false, then loop will end.

synchronized (someObject) {
    while (someObject.someCondition() == false && running) {
        try {
            someObject.wait(1000L);
        } catch (InterruptedException e) {
            e.printStackTrace();
    }
}

NOTE: This approach may not be the best in high performance threads, but I found it helpful in lot of situations.

BTW - if the JVM is executing a logic that is basically non-blocking (non-interruptible) such as the while loop condition in you code, then the interrupt fired by halt method will be 'lost'. And you would have to rely upon the 'isInterrupted' flag to tell the logic whether interrupt was invoked or not.