Timer/timertask to destroy a process that overruns a defined time limit

I was wondering if this is the correct implementation of a times task to destroy a thread after it overruns a predefined time period:

It works by creating a getting the thread from runtime.getRuntime, which it then uses to create a new process to run a command with the exec() method (creates a sub process). This is the process I want to terminate if it overruns. The code then handles the outputs streams from the process. before using waitFor() to let it finish running. However, in the circumstance it overruns, I have added a timer instance which I want it to use to terminate if the process overruns a defined time (retrieved using getTimeout()). However, I am worried the way I have done it will stop the original code from running.

Here is the timer code:

timer = new Timer();
timer.schedule(new TimerTask() {
@Override
public void run(){
      processWasKilledByTimeout = true;
      if(process != null){
          process.destroy();
       }
}
}, getTimeout();

I call this just after calling the exec method to create the subprocess.

Here is the full method:

```
private Process process;
private boolean processWasKilledByTimeout = false;

public String executeCommand()throws Exception {

InputStreamReader inputStreamReader = null;
InputStreamReader inputErrorStreamReader = null;

BufferedReader bufferedReader = null;
BufferedReader bufferedErrorReader = null;

StringBuffer outputTrace = new StringBuffer();
StringBuffer errorTrace = new StringBuffer();

String templine = null;

Timer timer = null;

Runtime runtime = Runtime.getRuntime();

try {

// Running exec produces one of two streams- combine to produce common string.
process = runtime.exec(getCommand());

timer = new Timer();
timer.schedule(new TimerTask() {
@Override
public void run(){
processWasKilledByTimeo

The main idea looks ok for me. Use a timer with a given time to call the destroy.

I would separate both concerns. One class is responsible for external program handling and it will provide a stop method. The stop method could be called from outside (e.g. the timer, but from manual interaction, too). The stop method will destroy the process and clean up.

The exact implementation depends if you either want to call it asynchronous (call, let it run, do something else, get a callback) or synchronous (call, wait for finish, continue). But the main flow would be the same.

I would not throw an exception (and even if, not with this hide and rethrow in other exception way). It is an expected behavior, not unexpected. So either call the corresponding callback function or provide a hasFinished() method or something like that and the caller could use it.

I think your output-reading would not work in all cases. You have to take care about both channels, stdout and stderr together, not one after the other. You could do this in one loop if you want, but the better way is to read them with asynchronous running threads. This shows the concept.

StackExchange Code Review Q#24907, answer score: 3