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

Question

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(){
                          processWasKilledByTimeout = true;
                          if(process != null){
                              process.destroy();
                           }
                    }
                    }, getTimeout();

        // The normal output stream
        InputStream inputStream = process.getInputStream();
        inputStreamReader = new InputStreamReader(inputStream);
        bufferedReader = new BufferedReader(inputStreamReader);     
        while (((templine = bufferedReader.readLine()) != null) && (!processWasKilledByTimeout)) {
            outputTrace.append(templine);
            outputTrace.append("\n");
        }
        this.setStandardOut(outputTrace);

        // The error output stream
        InputStream inputErrorStream = process.getErrorStream();
        inputErrorStreamReader = new InputStreamReader(inputErrorStream);
        bufferedErrorReader = new BufferedReader(inputErrorStreamReader);
        while (((templine = bufferedErrorReader.readLine()) != null) && (!processWasKilledByTimeout)) {
            errorTrace.append(templine);
            errorTrace.append("\n");
        }   
        this.setErrorTrace(errorTrace);

        //Wait for process to finish and return a code
        int returnCode = process.waitFor(); 

        //Set the return code
        this.setReturnCode(returnCode); 

        if (processWasKilledByTimeout) {
            //As process was killed by timeout just throw an InterruptedException
            throw new InterruptedException("Process was killed before the waitfor was reached.");
        }

    } catch(IOException ioe) {
        String error = "Error executing this command - "+getCommand() +". "+ioe.getMessage();
        throw new CommandExecuterException(error);
    }catch(IllegalArgumentException ile){
        String error = "Illegal argument encountered while executing this command - "+getCommand() +". "+ile.getMessage();
        throw new CommandExecuterException(error);
    }  catch(InterruptedException ie) {

                     error = "Process was killed after timeout by watchdog. "+ie.getMessage()+". ";
        throw  new CommandExecuterException(error);

    } finally {

        //cancel the timer not needed anymore
        if(timer!= null){
            timer.cancel();
        }


        try {

            if(bufferedReader != null){ 
                bufferedReader.close();
            }
            if(bufferedErrorReader != null){
                bufferedErrorReader.close();    
            }
            if(inputStreamReader != null){      
                inputStreamReader.close();
            }
            if(inputErrorStreamReader != null){
                inputErrorStreamReader.close();
            }

        } catch(IOException ioe) {

            String error = "Error closing stream - "+getCommand() +". "+ioe.getMessage();
            throw new CommandExecuterException(error);
        }


        if(mProcess != null) {

            mProcess.destroy();         
        }
    }

    //Assemble both the standardout and the errortrace
    String tmpReturnString = getStandardOut().toString()+"\n"+getErrorTrace().toString();

    return tmpReturnString; 
}

Will this implementation override the threads run method?

Answer 1

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.