Search code examples
javamultithreadinginterrupt

Is it a good way to close a thread?

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...

Solution

  • 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.