Is this a correct way to stop Execution Task

I came across code to stop execution's task.

private final ExecutorService executor = Executors.newSingleThreadExecutor();

public void stop() {
    executor.shutdownNow();
    try {
        executor.awaitTermination(100, TimeUnit.DAYS);
    } catch (InterruptedException ex) {
        log.error(null, ex);
    }
}


public Runnable getRunnable() {
    return new Runnable() {
        public void run() {
            while (!Thread.currentThread().isInterrupted()) {
                // What if inside fun(), someone try to clear the interrupt flag?
                // Say, through Thread.interrupted(). We will stuck in this loop
                // forever.
                fun();
            }
        }
    };
}

I realize that, it is possible for Runnable to be in forever loop, as

1. Unknown fun may Thread.sleep, clear the interrupt flag and ignore the InterruptedException
2. Unknown fun may Thread.interrupted, clear the interrupt flag.

I was wondering, is the follow开发者_StackOverflow社区ing way correct way to fix the code?

private final ExecutorService executor = Executors.newSingleThreadExecutor();
private volatile boolean flag = true;

public void stop() {
    flag = false;
    executor.shutdownNow();
    try {
        executor.awaitTermination(100, TimeUnit.DAYS);
    } catch (InterruptedException ex) {
        log.error(null, ex);
    }
}


public Runnable getRunnable() {
    return new Runnable() {
        public void run() {
            while (flag && !Thread.currentThread().isInterrupted()) {
                // What if inside fun(), someone try to clear the interrupt flag?
                // Say, through Thread.interrupted(). We will stuck in this loop
                // forever.
                fun();
            }
        }
    };
}

There is no way to ensure that an unknown function ends. If fun looks like the following you will be unable to terminate it without shutting down your application.

  void fun(){
     while(true){
        try{
           ...
        }catch(Throwable th){

        }
     }
  } 

Adding a flag to your Runnable wont help here since fun() will never exit. Several methods meant to terminate Threads like Thread.destroy() where deprecated since it would leave programs in an unknown state.

There are only two ways to ensure a function terminates:

• ensure that it follows a specified behavior. For you this would be: do not loop forever, don't clear the interrupted flag or your own flag.
• or, run the function in a separate instance of the jvm, you can kill the jvm if the function does not return without terminating your main program.

If you have control over fun() or know that it will return eventually, then adding your own flag would be a valid solution you should make sure that it deals with interrupts as you expect.

If I understand your question correctly:

• fun is supplied by a third party and you do not trust it (or you have reason to suspect that it is badly written.)
• You are not able to modify or replace fun.
• You want to protect your code from (intentional or unintentional) denial-of-service by the untrusted method.

This is not possible.
fun may be failing to terminate because it is silently clearing interrupts, but then it may be failing to terminate because of an infinite loop, or it may be doing something else harmful, e.g. calling System.exit or Thread.suspend.

If the only problem is silent clearing of interrupts (or something else that allows fun to return normally), then your flag will fix it, but I would recommend using executor.isShutDown() instead.

I think this is exactly what is documented in the javadoc of the executor service, isn't it?

You can use Thread.interrupted() if you don't need the flag to be set after the loop end. If its the only way to end the loop your code should assume it was interrupted (and if you have no code after your loop, you need not worry)

If your thread is running untrusted code, the only safe way to stop it is to run it in a separate process. Failing that you can interrupt it finally stop() it. stop() has many undesirable side effects (see the javadoc for it), but it may be preferable to leaving the thread running.