Destroying a process if it takes too much time

Question

I am executing a shell script from my Java program using Process and I want to kill/destroy that process if my script takes long time.

I came up with this code and wanted to see if there is any better way or anything I can improve on.

public class Test {

    public static void main(String[] args) throws IOException, InterruptedException {

        // Your script
        String script = getScriptTextFromSomewhere();

        // create a temp file and write your script to it
        File tempScript = File.createTempFile("temp_scripts_", "");
        tempScript.setExecutable(true);
        try (OutputStream output = new FileOutputStream(tempScript)) {
            output.write(script.getBytes());
        }

        // build the process object and start it
        List<String> commandList = new ArrayList<>();
        commandList.add(tempScript.getAbsolutePath());
        ProcessBuilder builder = new ProcessBuilder(commandList);
        builder.redirectErrorStream(true);
        builder.environment().put("param1", "abc");
        builder.environment().put("param2", "xyz");
        Process shell = builder.start();

        // read the output and show it
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(shell.getInputStream()))) {
            String line;
            while ((line = reader.readLine()) != null) {
                System.out.println(line);
            }
        }

        // wait for the process to finish
        WaitingThread thread = new WaitingThread(shell);
        thread.start();
        long timeoutMillis = 20000;
        thread.join(timeoutMillis);
        if(thread.isAlive()) {
            thread.interrupt();
        }

        if(!thread.isFinished()) {
            shell.destroy();
            shell.waitFor();
            System.out.println("PROCESS WAS INTERRUPTED");
        } else {
            // check the exit code (exit code = 0 usually means "executed ok")
            System.out.println("PROCESS FINISHED, EXIT CODE: " + thread.getExitValue());
        }

        // delete temp file
        tempScript.delete();
    }
}

class WaitingThread extends Thread {

    private int exitValue;
    private Process shell;
    private boolean finished = false;

    public WaitingThread(Process shell) {
        this.shell = shell;
    }

    @Override
    public void run() {
        try {
            exitValue = shell.waitFor();
            finished = true;
        } catch (InterruptedException e) {
            finished = false;
        }
    }

    public int getExitValue() {
        return exitValue;
    }

    public boolean isFinished() {
        return finished;
    }

}

Answer 1

First, I'm not too sure how your code works... your timeout starts after you begin to read the stdout of the spun Process object for non-null lines, but if you read the Javadoc of BufferedReader.readLine() again:

Returns:

A String containing the contents of the line, not including any line-termination characters, or null if the end of the stream has been reached

If my understanding of how the stdout pipelining done by the Process object is correct (wells, this being OS-dependent and what-not), I think the end of Process.getInputStream() is reached when the native process is disconnected. In most scenarios, said process would have completed too, unless we are talking about starting daemon processes.

In other words, the timeout check becomes a dummy step when it is reached, and thread.isFinished() will most likely return true, always.

Second, since you are already on Java 7 at least, you should be relying on a combination of ExecutorService and Future implementations to achieve your goal as they arguably have better semantics for performing timeout checks.

For example, since all you need is to run one background thread, you can use Executors.newSingleThreadExecutor():

ExecutorService service = Executors.newSingleThreadExecutor();

Then, since you do not require a return value, you can submit() a Runnable, and call the resulting Future's get(long, TimeUnit) method:

Runnable runnable = createRunnableForProcessHandling(shell);
Future<?> future = service.submit(runnable);
try {
    future.get(timeout, timeoutUnit);
    // completed within timeout
} catch (TimeoutException e) {
    // do something about the timeout
} finally {
    service.shutdown();
}

The alternative is to submit() a Callable to get perhaps the output of the native process as a List, for further processing:

Callable<List<String>> callable = getOutputLinesFrom(shell);
Future<List<String>> future = service.submit(callable);
try {
    List<String> outputLines = future.get(timeout, timeoutUnit);
    // do something with outputLines
} catch (TimeoutException e) {
    // do something about the timeout
} finally {
    service.shutdown();
}