Batch file process killed in Java: xcopy does not close

Question

This seems like it should be fairly easy to solve, but I am not familiar enough with the use of batch files to solve it for myself. I have a Java method that creates a process builder and runs a batch file in the process. The batch file uses the xcopy command to copy one directory to another. While the batch file is running in the background, a Java window containing a JTextArea displays the output of the process (the directories being copied). The window also has a stop button, which calls the following code:

stopped = true;
backgroundTask.cancel(true);
backgroundTask.done();

The done method looks like this:

protected void done() {
    statusLabel.setText((this.getState()).toString() + " " + status);
    stopButton.setEnabled(false);
    bar.setIndeterminate(false);
    if(stopped == false){
        JOptionPane.showMessageDialog(null, "Backup Complete.");
        closeWindow();
    }
    else if (stopped == true){
        JOptionPane.showMessageDialog(null, "Backup Cancelled.");
        closeWindow();
    }
}

Now, in order to run the batch file in the background, I use the following code (initially suggested to me by trashgod):

protected Integer doInBackground() throws IOException {
    try {
        ProcessBuilder pb = new ProcessBuilder(commands);
        pb.redirectErrorStream(true);
        Process p = pb.start();
        String s;
        BufferedReader stdout = new BufferedReader(
        new InputStreamReader(p.getInputStream()));
        while ((s = stdout.readLine()) != null && !isCancelled()) {
            publish(s);
        }
        if (!isCancelled()) {
            status = p.waitFor();
        }
        p.getInputStream().close();
        p.getOutputStream().close();
        p.getErrorStream().close();
        p.destroy();
        closeWindow();
    } catch (IOException | InterruptedException ex) {
        ex.printStackTrace(System.err);
    }            
    return status;
}

The issue I am having is this: When I run the program, the files copy just fine unless I press the stop button on the window in the foreground. When I do that it tells me the backup was cancelled (as it is supposed to), but it leaves three extra processes running which are visible in the task manager:

My guess is that the first one--the "extended copy utility" is the culprit. Since it's not closing, it leaves the other two cmd processes running. However, this is a fairly uneducated guess.

When I run the program and then stop it, windows explorer becomes very unstable, sometimes freezing, and sometimes crashing altogether. Navigation through folders--especially the directories being copied to--is extremely slow, and it appears that the directories continue to be copied even after the process is (supposedly) stopped. I believe it is because these lines are never reached:

p.getInputStream().close();
p.getOutputStream().close();
p.getErrorStream().close();
p.destroy();

so the process is never killed. I'm still working on a way to kill the processes completely when the stop button is pressed, but if anyone has thoughts I'd gladly hear them!

EDIT

I have opted to post the entire class since only giving certain methods probably doesn't give enough information. Here's the entire class:

package diana;

import java.awt.BorderLayout;
import java.awt.EventQueue;
import java.awt.Toolkit;
import java.awt.event.*;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.List;

import javax.swing.*;

@SuppressWarnings("serial")
public class Progress extends JFrame {
    public String[] commands;
    private final JLabel statusLabel = new JLabel("Status: ", JLabel.CENTER);
    private final JTextArea textArea = new JTextArea(20, 20);
    private JButton stopButton = new JButton("Stop");
    private JProgressBar bar = new JProgressBar();
    private BackgroundTask backgroundTask;
    private ProcessBuilder pb;
    private Process p;
    public boolean stopped = false;

    public void setCommands(String[] cmds) {
        commands = cmds;
    }
    private final ActionListener buttonActions = new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent ae) {
            JButton source = (JButton) ae.getSource();
            if (source == stopButton) {
                stopped = true;
                backgroundTask.cancel(true);
                backgroundTask.done();
            } else {
                backgroundTask = new BackgroundTask(commands);
            }
        }
    };

    private void displayGUI(String[] cmds) {
        commands = cmds;
        JFrame frame = new JFrame("Backup Progress");
        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        JPanel panel = new JPanel();
        panel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));
        panel.setLayout(new BorderLayout(5, 5));
        JScrollPane sp = new JScrollPane();
        sp.setBorder(BorderFactory.createTitledBorder("Output: "));
        sp.setViewportView(textArea);
        textArea.setText(null);
        stopButton.setEnabled(true);
        backgroundTask = new BackgroundTask(commands);
        backgroundTask.execute();
        bar.setIndeterminate(true);
        stopButton.addActionListener(buttonActions);
        JPanel buttonPanel = new JPanel();
        buttonPanel.add(stopButton);
        buttonPanel.add(bar);
        panel.add(statusLabel, BorderLayout.PAGE_START);
        panel.add(sp, BorderLayout.CENTER);
        panel.add(buttonPanel, BorderLayout.PAGE_END);
        frame.setContentPane(panel);
        frame.pack();
        frame.setLocationByPlatform(true);
        frame.setVisible(true);
    }

    /* Close current window */
    public void closeWindow() throws IOException {
        p.getInputStream().close();
        p.getOutputStream().close();
        p.getErrorStream().close();
        p.destroy();
        WindowEvent close = new WindowEvent(this, WindowEvent.WINDOW_CLOSING);
        Toolkit.getDefaultToolkit().getSystemEventQueue().postEvent(close);
        System.exit(0);
    }

    private class BackgroundTask extends SwingWorker<Integer, String> {
        private int status;
        public String[] commands;
        public BackgroundTask(String[] cmds) {
            commands = cmds;
            statusLabel.setText((this.getState()).toString());
        }

        @Override
        protected Integer doInBackground() throws IOException {
            try {
                pb = new ProcessBuilder(commands);
                pb.redirectErrorStream(true);
                p = pb.start();
                String s;
                BufferedReader stdout = new BufferedReader(
                    new InputStreamReader(p.getInputStream()));
                while ((s = stdout.readLine()) != null && !isCancelled()) {
                    publish(s);
                }
                if (!isCancelled()) {
                    status = p.waitFor();
                }
                closeWindow();
            } catch (IOException | InterruptedException ex) {
                ex.printStackTrace(System.err);
            }
            return status;
        }

        @Override
        protected void process(List<String> messages) {
            statusLabel.setText((this.getState()).toString());
            for (String message : messages) {
                textArea.append(message + "\n");
            }
        }

        @Override
        protected void done() {
            statusLabel.setText((this.getState()).toString() + " " + status);
            stopButton.setEnabled(false);
            bar.setIndeterminate(false);
            if (stopped == false) {
                JOptionPane.showMessageDialog(null, "Backup Complete.");
                try {
                    closeWindow();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            } else if (stopped == true) {
                JOptionPane.showMessageDialog(null, "Backup Cancelled.");
                try {
                    closeWindow();
                } catch (IOException e) {
                    e.printStackTrace();
                }
            }
        }
    }

    public void run(String[] cmds) {
        commands = cmds;
        EventQueue.invokeLater(new Runnable() {
            @Override
            public void run() {
                new Progress().displayGUI(commands);
            }
        });
    }
}

Once again, I cannot take credit for this code, as it is mainly provided by SO member trashgod. Also, please excuse any statements left in there for debugging that I may have forgotten to remove.

Answer 1

One of my thoughts is that it is unreasonable to expect to be able to stop this overall process. You are starting up a shell and handing it a command, then stopping the original program -- what is it supposed to do to stop the copying? If you were a user using a command shell you could enter control-C, but I don't know whether there is a programming equivalent, available to Java, that will do the same thing.

Answer 2

There are several things that stand out

BufferedReader#readLine is a blocking method and may not respond to the interrupted flag of the current thread (and unblock)

You've surrounded your entire logic in a single try-catch blocking, meaning that if InterruptedException is thrown, you skip the entire portion of the code that you are trying to use to dispose of the process.

Something like this might be a slightly better approach. InputStream#read is still your Achilles heel, but because I'm now check isCancelled BEFORE trying to read something, it's less likely to cause a great deal of issue

InputStream is = null;
Process p = null;
try {
    ProcessBuilder pb = new ProcessBuilder(commands);
    pb.redirectErrorStream(true);
    p = pb.start();

    StringBuilder sb = new StringBuilder(128);
    is = p.getInputStream();
    int in = -1;
    while (!isCancelled() && (in = is.read()) != -1) {
        sb.append((char)in));
        if (((char)in) == '\n') {
            publish(sb.toString());
            sb.delete(0, sb.length());
        }
    }
    if (!isCancelled()) {
        status = p.waitFor();
    } else {
        p.destroy();
    }
} catch (IOException ex) {
    ex.printStackTrace(System.err);
} catch (InterruptedException ex) {
    ex.printStackTrace(System.err);
    try {
        p.destroy();
    } catch (Exception exp) {
    }
} finally {
    try {
        is.close();
    } catch (Exception exp) {
    }
    // Make sure you are re-syncing this to the EDT first...
    closeWindow();
}

(nb Typed directly, so I've not tested it)

Answer 3

You didn't say whether your batch file performs any actions in addition to calling xcopy. If not, you may want to consider using Java to do the file copy instead of running an external process. It's a lot easier to interrupt your own code than to stop an external process:

static void copyTree(final Path source, final Path destination)
throws IOException {
    if (Files.isDirectory(source)) {
        Files.walkFileTree(source, new SimpleFileVisitor<Path>()
        {
            @Override
            public FileVisitResult preVisitDirectory(Path dir,
                                         BasicFileAttributes attributes)
            throws IOException {
                if (Thread.interrupted()) {
                    throw new InterruptedIOException();
                }

                Path destinationDir =
                    destination.resolve(source.relativize(dir));
                Files.createDirectories(destinationDir);

                BasicFileAttributeView view =
                    Files.getFileAttributeView(destinationDir,
                        BasicFileAttributeView.class);
                view.setTimes(
                    attributes.lastModifiedTime(),
                    attributes.lastAccessTime(),
                    attributes.creationTime());

                return FileVisitResult.CONTINUE;
            }

            @Override
            public FileVisitResult visitFile(Path file,
                                         BasicFileAttributes attributes)
            throws IOException {
                if (Thread.interrupted()) {
                    throw new InterruptedIOException();
                }

                Files.copy(file,
                    destination.resolve(source.relativize(file)),
                    StandardCopyOption.COPY_ATTRIBUTES,
                    LinkOption.NOFOLLOW_LINKS);

                return FileVisitResult.CONTINUE;
            }
        });
    } else {
        Files.copy(source, destination,
            StandardCopyOption.COPY_ATTRIBUTES,
            LinkOption.NOFOLLOW_LINKS);
    }
}