3

I have a Java book I'm learning from and in one of the examples, I saw something suspicious.

public class ThreadExample extends MIDlet {
    boolean threadsRunning = true; // Flag stopping the threads

    ThreadTest thr1;
    ThreadTest thr2;

    private class ThreadTest extends Thread {
        int loops;

        public ThreadTest(int waitingTime) {
            loops = waitTime;
        }

        public void run() {
            for (int i = 1; i <= loops; i++) {
                if (threadsRunning != true) { // here threadsRunning is tested
                    return;
                }

                try {
                    Thread.sleep(1000);
                } catch(InterruptedException e) {
                    System.out.println(e);
                }
            }
        }
    }

    public ThreadExample() {
        thr1 = new ThreadTest(2);
        thr2 = new ThreadTest(6);
    }

    public void startApp() throws MIDletStateChangeException {
        thr1.start();
        thr2.start();

        try {
            Thread.sleep(4000); // we wait 4 secs before stopping the threads - 
                                // this way one of the threads is supposed to finish by itself
        } catch(InterruptedException e) {
            System.out.println(e);
        }

        destroyApp();
    }

    public void destroyApp() {    
        threadsRunning = false;

        try {
            thr1.join();
            thr2.join();
        } catch(InterruptedException e) {
            System.out.println(e);
        }

        notifyDestroyed();
    }
}

As it is a MIDlet app, when it's started, the startApp method is executed. To keep it simple, the startApp method itself calls destroyApp and so the program destroys, stopping the threads and notifying the destruction.

The question is, is it safe to use this 'threadsRunning' variable and would its use inside both threads and in the destroyApp method cause any trouble at some point? Would 'volatile' keyword put in front of the declaration help to synchronize it?

gnat
  • 6,213
  • 108
  • 53
  • 73
closer
  • 43
  • 1
  • 6
  • Als long as you just read this variable inside the threads it should be save – Matthias Bruns Nov 09 '11 at 10:17
  • Thank you. So, only if I Write to a variable on more than one thread, then it's dangerous without using synchronization mechanism, right? – closer Nov 09 '11 at 10:36
  • 1
    There are cases where writing to a variable from more than one thread is safe, but in most cases it is not. There are also cases when writing to a variable from just one thread and reading it from others is safe, and cases where it is not. It gets so complicated, I usually just synchronise access to any shared variable. – Joe Daley Nov 09 '11 at 10:51
  • just a note: if the Thread gets an InterruptedException it should stop - that is, at least exit the loop in above code. – user85421 Nov 09 '11 at 13:51

1 Answers1

6

Setting a boolean value is atomic, and there is no "read then modify" logic in this example, so access to the variable doesn't need to be synchronised in this particular case.

However, the variable should at least be marked volatile.

Marking the variable volatile does not synchronise the threads' access to it; it makes sure that a thread doesn't miss another thread's update to the variable due to code optimisation or value caching. For example, without volatile, the code inside run() may read the threadsRunning value just once at the beginning, cache the value, and then use this cached value in the if statement every time, rather than reading the variable again from main memory. If the threadsRunning value gets changed by another thread, it might not get picked up.

In general, if you use a variable from multiple threads, and its access is not synchronised, you should mark it volatile.

Voo
  • 29,040
  • 11
  • 82
  • 156
Joe Daley
  • 45,356
  • 15
  • 65
  • 64
  • Updated my answer, because I changed my mind. Although this particular example might not need it due to a technicality, when using a variable this way it should be marked volatile. – Joe Daley Nov 09 '11 at 13:34
  • I removed the part about the memory barrier, because JLS 17.9 says the following: `It is important to note that neither Thread.sleep nor Thread.yield have any synchronization semantics. In particular, the compiler does not have to flush writes cached in registers out to shared memory before a call to Thread.sleep or Thread.yield, nor does the compiler have to reload values cached in registers after a call to Thread.sleep or Thread.yield.` – Voo Nov 09 '11 at 13:40
  • @Voo Nice! I rechecked my source that said Thread.sleep was a memory barrier, and it was about .NET not Java. Thanks for the correction. – Joe Daley Nov 09 '11 at 13:47
  • @JoeDaley follow up question.. What if inside the `run()` will create a new `runnable`. how can that new thread read the `threadsRunning` if the `runnable` is not a nested class. Could i just pass the `threadsRunning`in the constructor of the new `runnable`? I see it's working but is it advisable? – Erik Nov 26 '11 at 21:45