0

I have a little trouble with stopping the thread. I have method for scanning files in a folder. I want to have button which stop this scanning. After hitting this button thread will stop (scanning will stop) and program will reset. I've tried command thread.stop(); and it worked sometimes (It's not working anymore). So I've read some topics here on stackoverflow and tried thread.interrupt(); My code then looked like this:

public class myApp extends javax.swing.JFrame {
  Thread thread;
  int fileCount = 0;
  String path = "C:\\Program Files";

    public void scanningMethod() {
     thread = new Thread(new Runnable() {
        public void run() {
         while(!thread.interrupted()){
             //Recursion method that counting files
             dirScan(path);
           }  
        }    
     });
   thread.start();
  }

  private void stopButtonActionPerformed(java.awt.event.ActionEvent evt) {
      //generated button in netBeans
      thread.interrupt();
      thread.stop();
   }

 private void dirScan(String dirPath) {
    File[] podSoubory = new File(dirPath).listFiles();

    for (int i = 0; i < podSoubory.length; i++) {
        if (podSoubory[i].isDirectory()) {
            String tempPath = podSoubory[i].getAbsolutePath();
            System.out.println(tempPath);

        if (podSoubory[i].isFile()) {
            fileCount++;
        }
    }
  }
}

Another method for stop contained only thread.interrupt(); (ofc it has actionListener and stuff).

Probably I'm doing something wrong. Can you help me and tell me how to stop this running thread after clicking on the button? (I know how to create clickable button).

Hovercraft Full Of Eels
  • 283,665
  • 25
  • 256
  • 373
Bukk94
  • 419
  • 1
  • 6
  • 23
  • 4
    To help us fully understand your problem, please create and post your [mcve]. This is not your complete program, which would likely be too much for us, but is a compilable and runnable, ***minimal*** example program, one that requires no outside resources (such as files or images), or if it does require these resources that are easily available to us, one that we can run and modify and that demonstrates your error for us. – Hovercraft Full Of Eels Aug 12 '15 at 21:07
  • 1
    How can you interrupt the thread when it's declared locally within a method? – MadProgrammer Aug 12 '15 at 21:10
  • Also note, 1) I don't see how your code as posted can get a reference to the thread of interest and thereby call `thread.interrupt()`, 2) if this is being called from a Swing GUI, consider using a SwingWorker to help generate your background thread as this allows your background code to work well with your Swing GUI. Also note that SwingWorker has a `cancel()` method that could be useful for your purposes. And 3) I'm sure you've read not to use `Thread#stop()`, that this is dangerous code -- and I'm here to tell you that what you've read is true -- don't call this method. – Hovercraft Full Of Eels Aug 12 '15 at 21:11
  • My program is pretty messy right now. My problem is that running thread won't stop after calling thread.stop(); or thread.interrupt(); But I'll add link on github. – Bukk94 Aug 12 '15 at 21:12
  • `But I'll add link on github.` -- which is not what we've asked for or need. Again, we don't want to see the whole messy program, as you'd be asking way too much effort and work from volunteers which is what we are. Please read or re-read the link in my first post. Post your pertinent [mcve] code **here** with your question. – Hovercraft Full Of Eels Aug 12 '15 at 21:14
  • Sorry, but -1 for ignoring recommendations. Please help us help you. – Hovercraft Full Of Eels Aug 12 '15 at 21:17
  • I'm not ignoring your recommendations. I'm trying to edit it that you can read it better. But I can't do it within 2 mins. – Bukk94 Aug 12 '15 at 21:22
  • I can tell you that your current code is dangerous in that it calls `stop()` on threads and in that it ignores Swing threading rules as you appear to be making unsafe Swing calls directly from within backgound threads. Again, a SwingWorker would help you with this. Another issue is that your class is one huge "God Class", a class that tries to do everything, and this is why your code is a "real mess", hard to refactor and hard to debug. I strongly urge you to refactor to separate unrelated concerns. get your threading and your file manipulation into their own ... – Hovercraft Full Of Eels Aug 12 '15 at 21:26
  • ... separately testable classes, which will make your job of debugging, and later of enhancing and improving **much** easier. – Hovercraft Full Of Eels Aug 12 '15 at 21:26
  • Also,... you've at least one call to `Thread.sleep(1000)` that will swallow and ignore an InterruptedException, which may be contributing to your problem. If the exception occurs, rather than print System.err, consider breaking out of the scanning method. – Hovercraft Full Of Eels Aug 12 '15 at 21:30
  • OK, that's about all I can do with the material provided. Please comment me and @MadProgrammer back when you've created and posted your [mcve] so that we can help figure out what is wrong. – Hovercraft Full Of Eels Aug 12 '15 at 21:33
  • I know it's dangerous to call `stop();` that's why I tried to call `interrupt()`. When it doesn't worked i've tried `stop()`. I'm working with java 3rd month now so I can say that I'm beginner and this is my first attempt with Threads (I know this is not excuse) but I know where are all things so it's not mess for me. I thought it'll be easier with threads. I've underestimate it. – Bukk94 Aug 12 '15 at 21:33
  • Again, fix that `Thread.sleep(1000)` error to see what happens. It may be swallowing your interrupt. – Hovercraft Full Of Eels Aug 12 '15 at 21:35
  • It has to be something else. I've removed whole method with `Thread.sleep` and I can't stop it. Am I calling `interrupt()` method right? – Bukk94 Aug 12 '15 at 21:39
  • I don't see an error there, but can't tell based on the size and cyclomatic complexity of your program. I've removed the down-vote in anticipation of your creating and posting here a simple runnable version of the pertinent code. Again, please let us know when it is available to us. – Hovercraft Full Of Eels Aug 12 '15 at 21:43

3 Answers3

3

As to your main question, that of why your code is not working, I'm sorry, but I don't have an answer, since unfortunately, I don't have the time to go through and analyze all of your code. I will say though that yours is dangerous code (as you know), not only due to your calling Thread#stop() but also due to your code's not respecting Swing threading rules, rules which require that most all Swing calls be made on the Swing event thread. You are calling some key Swing methods, including setText(...) methods within background threads, and this risks causing intermittent very hard to debug threading errors.

I suggest:

  • Use SwingWorkers for your background thread, for several reasons, but mainly so you can publish/process data from the background thread safely into the GUI.
  • If you want to kill your SwingWorker, call its cancel(true) method, passing in a true parameter to allow this call to cancel the running code.
  • Within your SwingWorker, check the isCancelled() state.
  • Any text or data that you want the background thread should be passed into a publish(...) call within your SwingWorker's doInBackground() method.
  • The SwingWorker can then handle this data within its process(...) method, a method which is guaranteed to be called on the Swing event thread.

For example:

import java.awt.BorderLayout;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.beans.PropertyChangeEvent;
import java.beans.PropertyChangeListener;
import java.util.List;
import java.util.concurrent.ExecutionException;

import javax.swing.*;

public class InterruptedGui {
    private static final int GAP = 3;
    private MyWorker myWorker;
    private StartWorkerAction startWorkerAxn = new StartWorkerAction(this,
            "Worker");
    private StopAllAction stopAllAction = new StopAllAction(this, "Stop All");
    private JLabel statusLabel = new JLabel("");
    private JTextArea finalTextArea = new JTextArea(20, 40); 
    private JPanel mainPanel = new JPanel();

    public InterruptedGui() {
        finalTextArea.setFocusable(false);
        JPanel buttonPanel = new JPanel(new GridLayout(1, 0, GAP, 0));
        buttonPanel.add(new JButton(startWorkerAxn));
        buttonPanel.add(new JButton(stopAllAction));

        JPanel statusPanel = new JPanel();
        statusPanel.setLayout(new BoxLayout(statusPanel, BoxLayout.LINE_AXIS));
        statusPanel.add(new JLabel("Status: "));
        statusPanel.add(statusLabel);

        mainPanel.setBorder(BorderFactory.createEmptyBorder(GAP, GAP, GAP, GAP));
        mainPanel.setLayout(new BorderLayout());
        mainPanel.add(buttonPanel, BorderLayout.PAGE_START);
        mainPanel.add(new JScrollPane(finalTextArea,
                JScrollPane.VERTICAL_SCROLLBAR_ALWAYS,
                JScrollPane.HORIZONTAL_SCROLLBAR_AS_NEEDED));

        mainPanel.add(statusPanel, BorderLayout.PAGE_END);
    }

    public JComponent getMainPanel() {
        return mainPanel;
    }

    public void setStatus(String text) {
        statusLabel.setText(text);
    }

    public MyWorker getMyWorker() {
        return myWorker;
    }

    public void setMyWorker(MyWorker myWorker) {
        this.myWorker = myWorker;
    }

    public void finalTextAreaSetText(String text) {
        finalTextArea.setText(text);
    }

    private static void createAndShowGui() {
        JFrame frame = new JFrame("Interrupted Gui");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().add(new InterruptedGui().getMainPanel());
        frame.pack();
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }

    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                createAndShowGui();
            }
        });
    }
}

class MyWorker extends SwingWorker<String, String> {
    private static final long SLEEP_TIME = 100;
    private static final int MAX_COUNTER = 100;
    private int counter = 0;
    private InterruptedGui gui;

    public MyWorker(InterruptedGui gui) {
        this.gui = gui;
    }

    @Override
    protected String doInBackground() throws Exception {
        StringBuilder sb = new StringBuilder();
        while (counter < MAX_COUNTER && !isCancelled()) {
            counter++;
            String statusText = "Counter is " + counter;
            sb.append(statusText + "\n");
            publish(statusText);
            Thread.sleep(SLEEP_TIME);
        }
        return sb.toString();
    }

    @Override
    protected void process(List<String> chunks) {
        for (String statusText : chunks) {
            gui.setStatus(statusText);
        }
    }
}

@SuppressWarnings("serial")
class StartWorkerAction extends AbstractAction {
    private InterruptedGui gui;

    public StartWorkerAction(InterruptedGui gui, String name) {
        super(name);
        this.gui = gui;
        int mnemonic = (int) name.charAt(0);
        putValue(MNEMONIC_KEY, mnemonic);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        MyWorker myWorker = gui.getMyWorker();
        if (myWorker != null && !myWorker.isDone()) {
            return; // still running current worker
        }
        gui.finalTextAreaSetText("");
        myWorker = new MyWorker(gui);
        gui.setMyWorker(myWorker);
        myWorker.addPropertyChangeListener(new WorkerPropertyListener(gui));
        myWorker.execute();
    }
}

class WorkerPropertyListener implements PropertyChangeListener {
    private InterruptedGui gui;

    public WorkerPropertyListener(InterruptedGui gui) {
        this.gui = gui;
    }

    @Override
    public void propertyChange(PropertyChangeEvent evt) {
        if (evt.getNewValue() == SwingWorker.StateValue.DONE) {
            MyWorker myWorker = gui.getMyWorker();
            if (myWorker != null && !myWorker.isCancelled()) {
                try {
                    String finalText = myWorker.get();
                    gui.finalTextAreaSetText(finalText);
                } catch (InterruptedException e) {
                    e.printStackTrace();
                } catch (ExecutionException e) {
                    e.printStackTrace();
                }
            }
        }
    }
}

@SuppressWarnings("serial")
class StopAllAction extends AbstractAction {
    private InterruptedGui gui;

    public StopAllAction(InterruptedGui gui, String name) {
        super(name);
        this.gui = gui;
        int mnemonic = (int) name.charAt(0);
        putValue(MNEMONIC_KEY, mnemonic);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        MyWorker myWorker = gui.getMyWorker();
        if (myWorker == null) {
            return;
        }
        myWorker.cancel(true);
    }
}
Hovercraft Full Of Eels
  • 283,665
  • 25
  • 256
  • 373
  • I tried to use Swing Worker before I've started, but I failed. I don't know much about it. Thanks for the example I'll check it. – Bukk94 Aug 13 '15 at 00:07
3

You have a misconception about how thread interruption works, when you call Thread#interrupt all that happens is a flag is raised within the Thread instance, which you can inspect using a interrupted or isInterrupted.

In your code, you have a for-loop

for (int i = 0; i < podSoubory.length; i++) {
    if (podSoubory[i].isDirectory()) {
        String tempPath = podSoubory[i].getAbsolutePath();
        System.out.println(tempPath);

    if (podSoubory[i].isFile()) {
        fileCount++;
    }
}

This means that until this for-loop exists, the while(!thread.interrupted()){ statement won't be evaluated.

What you need to do is test isInterrupted at periodalical points within your code, for example...

for (int i = 0; i < podSoubory.length && !Thread.currentThread().isInterrupted(); i++) {
    if (podSoubory[i].isDirectory()) {
        String tempPath = podSoubory[i].getAbsolutePath();
        System.out.println(tempPath);

        if (podSoubory[i].isFile()) {
            fileCount++;
        }
    }
}

This adds a check for isInterrupted, this is imported, as isInterrupted WON'T clear the interrupted flag like Thread#interrupted will, this allows other parts of your code to further test the interrupted flag as well.

When you interrupt your Thread, both the for-loop AND while-loop will check the state of the interrupted flag and will allow both loops to exit.

As a runnable example...

import java.awt.EventQueue;
import java.awt.GridBagLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.io.File;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.UIManager;
import javax.swing.UnsupportedLookAndFeelException;

public class MyApp {

    public static void main(String[] args) {
        new MyApp();
    }

    public MyApp() {
        EventQueue.invokeLater(new Runnable() {
            @Override
            public void run() {
                try {
                    UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
                } catch (ClassNotFoundException | InstantiationException | IllegalAccessException | UnsupportedLookAndFeelException ex) {
                    ex.printStackTrace();
                }

                JFrame frame = new JFrame("Testing");
                frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
                frame.add(new TestPane());
                frame.pack();
                frame.setLocationRelativeTo(null);
                frame.setVisible(true);
            }
        });
    }

    public class TestPane extends JPanel {

        public TestPane() {

            setLayout(new GridBagLayout());
            JButton stop = new JButton("Stop");
            stop.addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {
                    thread.interrupt();
                    // Join is used here to prove a point, be careful
                    // with using this within the context of the EDT
                    try {
                        thread.join();
                    } catch (InterruptedException ex) {
                        ex.printStackTrace();
                    }
                    stop.setEnabled(false);
                }
            });
            add(stop);

            scanningMethod();
        }

        Thread thread;
        int fileCount = 0;
        String path = "C:\\Program Files";

        public void scanningMethod() {
            thread = new Thread(new Runnable() {
                public void run() {
                    while (!thread.isInterrupted()) {
                        //Recursion method that counting files
                        dirScan(path);
                        System.out.println(thread.isInterrupted());
                    }
                }
            });
            thread.start();
        }

        private void dirScan(String dirPath) {
            File[] podSoubory = new File(dirPath).listFiles();

            for (int i = 0; i < podSoubory.length && !Thread.currentThread().isInterrupted(); i++) {
                if (podSoubory[i].isDirectory()) {
                    String tempPath = podSoubory[i].getAbsolutePath();
                    System.out.println(tempPath);

                    if (podSoubory[i].isFile()) {
                        fileCount++;
                    }
                }
            }

        }
    }

}

You might want to have a look at Concurrency in Java for more details

Also, Thread#stop is deprecated and should NEVER be used, from the JavaDocs...

Deprecated. This method is inherently unsafe. Stopping a thread with Thread.stop causes it to unlock all of the monitors that it has locked (as a natural consequence of the unchecked ThreadDeath exception propagating up the stack). If any of the objects previously protected by these monitors were in an inconsistent state, the damaged objects become visible to other threads, potentially resulting in arbitrary behavior. Many uses of stop should be replaced by code that simply modifies some variable to indicate that the target thread should stop running. The target thread should check this variable regularly, and return from its run method in an orderly fashion if the variable indicates that it is to stop running. If the target thread waits for long periods (on a condition variable, for example), the interrupt method should be used to interrupt the wait. For more information, see Why are Thread.stop, Thread.suspend and Thread.resume Deprecated?.

MadProgrammer
  • 343,457
  • 22
  • 230
  • 366
  • I thought that I can have only 1 condition in for loop. But I've got your point and idea. I'll try it tomorrow. Now I understand how it works and I see where's problem (probably). Thanks a lot! I'll comment here tomorrow how it went. – Bukk94 Aug 13 '15 at 00:04
  • Like I said, I can't use more than 1 condition in a loop, but I've added additional 2 if statements. They checked `!thread.isInterrupted()`. One was in the loop and one on the beginning of the method. But after calling `thread.interrupt()` loop still ran even when it should skip. Probably I have to rework it on SwingWorker. + I've removed `thread.stop()` – Bukk94 Aug 13 '15 at 13:10
-1

I would suggest to implement a thread through Runnable interface. In this case, you will need to implement run() and kill() methods. Introduce local shared variable as a flag. In kill method set the flag to false, in the run method perform the action until the flag is true.

In general, I would suggest to get familiar with wait/notify mechanism which might be helpful for you in this case as well as Java Executioner framework.

Most important, please search stackoverflow for similar questions, they been answered before. For example, Java: How to stop thread?.

Hope this helps.

Boris
  • 191
  • 6