Is it a good way to close a thread?

Question

I have a short version of the question:

1. I start a thread like that: counter.start();, where counter is a thread.
2. At the point when I want to stop the thread I do that: counter.interrupt()
3. In my thread I periodically do this check: Thread.interrupted(). If it gives true I return from the thread and, as a consequence, it stops.

---

And here are some details, if needed:

If you need more details, they are here. From the invent dispatch thread I start a counter thread in this way:

public static void start() {
    SwingUtilities.invokeLater(new Runnable() {
        public void run() {
            showGUI();
            counter.start();
        }
    });
}

where the thread is defined like that:

public static Thread counter = new Thread() {
    public void run() {
        for (int i=4; i>0; i=i-1) {
            updateGUI(i,label);
            try {Thread.sleep(1000);} catch(InterruptedException e) {};
        }
            // The time for the partner selection is over.
        SwingUtilities.invokeLater(new Runnable() {
                public void run() {    
                frame.remove(partnerSelectionPanel);
                frame.add(selectionFinishedPanel);
                frame.invalidate();
                frame.validate();
            }
        });
    }
};

The thread performs countdown in the "first" window (it shows home much time left). If time limit is over, the thread close the "first" window and generate a new one. I want to modify my thread in the following way:

public static Thread counter = new Thread() {
    public void run() {
        for (int i=4; i>0; i=i-1) {
            if (!Thread.interrupted()) {
                updateGUI(i,label);
            } else {
                return;
            }
            try {Thread.sleep(1000);} catch(InterruptedException e) {};
        }
        // The time for the partner selection is over.
        if (!Thread.interrupted()) {
            SwingUtilities.invokeLater(new Runnable() {
                public void run() {    
                frame.remove(partnerSelectionPanel);
                frame.add(selectionFinishedPanel);
                frame.invalidate();
                frame.validate();
            }
        });
        } else {
            return;
        } 
    }
};

---

ADDED:

Because of some reasons it does not work. I have a method which interrupts the thread:

public static void partnerSelected() {
    System.out.println("The button is pressed!!!!");
    counter.interrupt();
}

This method is activated when a button is pressed. When I press the button I see the corresponding output in the terminal (so this method is activated and it does something). But because of some reasons it does not interrupt the thread. Here is the code for the thread:

public static Thread counter = new Thread() {
    public void run() {
        for (int i=40; i>0; i=i-1) {
                if (Thread.interrupted()) {
                    System.out.println("Helloo!!!!!!!!!!!!!!!");
                    return;
                }
            updateGUI(i,label); 
            try {Thread.sleep(1000);} catch(InterruptedException e) {};
        }
            // The time for the partner selection is over.
            if (Thread.interrupted()) {
                System.out.println("Helloo!!!!!!!!!!!!!!!");
                return;
            }
        SwingUtilities.invokeLater(new Runnable() {
                public void run() {    
                frame.remove(partnerSelectionPanel);
                frame.add(selectionFinishedPanel);
                frame.invalidate();
                frame.validate();
            }
        });
    }
};

P.S. I do not see "Hello!!!!!!!!!!!!!" in the terminal...

Answer 1

Pretty close to the right idea. However, in your catch (InterruptedException) you should have:

Thread.currentThread().interrupt();

so that the interrupted status goes on again, and doesn't do the stuff in the second block.

---

Edit to make my point clearer (because the OP's edit seems to have missed my initial point :-P): you should write your code like this:

try {
    for (int = 40; i > 0; --i) {
        updateGUI(i, label);
        Thread.sleep(1000);
    }
} catch (InterruptedException e) {
    Thread.currentThread().interrupt();  // <-- THIS LINE IS IMPORTANT
}

---

Second edit to explain what interruption does. :-)

When you call thread.interrupt(), that thread's interrupted flag is set. That flag doesn't do anything on its own; it's just a variable. The reason for this is because interruption supports something called "cooperative thread management", where the thread's running code decides what to do when interrupted (rather than being forced to quit on the spot).

Some functions built into the JDK, like Thread.sleep, or Object.wait, or Lock.lockInterruptibly, will check the flag, and if it's set, then it'll throw an InterruptedException after clearing the flag.

So, if you're calling one of those functions, you don't need to manually check the interrupted flag. But if you're not, e.g., if you're doing intensive processing instead of waiting for something, then you should periodically check the flag.

There are two ways to check the flag:

1. interrupted()
2. isInterrupted()

The first one clears the interrupted flag; the second one doesn't. You have to decide which version is "more correct" for your application logic.

Answer 2

Check out this article from the JavaSpecialists newsletter, which covers how to interrupt() threads and manage this properly.

Answer 3

Yes it is the way to go

Answer 4

1. It's considered a better way (link) to use separate volatile variable (boolean isStopped) for this purpose.

2. Assume that interrupted() method changes value from true to false if your thread was interrupted, i.e.:

System.out.println (Thread.interrupted()); //true

System.out.println (Thread.interrupted()); //false

The alternative is isInterrupted() method.

Answer 5

I'd like to edit and note that I've learned a lesson here today. There's no reason to implement a boolean as I explain in the following two paragraphs; the interrupt mechanism does that for me. For some reason I had assumed that "interrupt" stops the thread dead in its tracks (I don't know what I thought isInterrupted() did then!).

So, here is an example of what not to do. Keep on using your interrupt technique!

(please don't downvote me...)

---

I tend to avoid interrupt, but especially to stop a thread. In your case, you're trying to use interrupt() as an alternative to stop(), which has been deprecated for good reason. All you need to do is declare a boolean which represents whether the thread should stop counting, and have the thread continuously check that boolean value. Then, when the parent thread is ready for the counter to stop, it should set the boolean to true (stop), which will cause the counter thread to stop as soon as it checks the value again.

In your Counter thread's anonymous class definition, add public volatile boolean shouldStop;. At the beginning of run(), set shouldStop = false;. Then replace all Thread.interrupted() with shouldStop (in your if statements). Finally, instead of calling counter.interrupt(), just say counter.shouldStop = true;. You can additionally call counter.join() right after setting shouldStop=true if you want to ensure that counter has stopped before continuing.