Can it be acceptable in Java to use Thread#stop() to kill a thread that is running wild?

Question

Regrettably there is no way to specify a timeout when using a regular expression on a String in Java. So if you have no strict control over what patterns get applied to which input, you might end up having threads that consume a lot of CPU while endlessly trying to match (not so well designed) patterns to (malicious?) input.

I'm aware of the reasons why Thread#stop() is deprecated (see http://download.oracle.com/javase/1.5.0/docs/guide/misc/threadPrimitiveDeprecation.html). They are centered around objects that might get damaged in case of ThreadDeath exceptions, and which then pollute your running JVM environment and can lead to subtle errors.

My question to anyone who has deeper insight than me into the workings of the JVM is this: If the thread that needs to be stopped does not hold any (obvious) monitors on or references to objects that are used by the rest of the program, can it then be acceptable to use Thread#stop() nevertheless?

I created a rather defensive solution to be able to process regular expression matching with a timeout. I would be glad for any comment or remark, especially on problems that this approach can cause despite my efforts to avoid them.

Thanks!

import java.util.concurrent.Callable;

public class SafeRegularExpressionMatcher {

    // demonstrates behavior for regular expression running into catastrophic backtracking for given input
    public static void main(String[] args) {
        SafeRegularExpressionMatcher matcher = new SafeRegularExpressionMatcher(
                "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", "(x+x+)+y", 2000);
        System.out.println(matcher.matches());
    }

    final String stringToMatch;

    final String regularExpression;

    final int timeoutMillis;

    public SafeRegularExpressionMatcher(String stringToMatch, String regularExpression, int timeoutMillis) {
        this.stringToMatch = stringToMatch;
        this.regularExpression = regularExpression;
        this.timeoutMillis = timeoutMillis;
    }

    public Boolean matches() {
        CallableThread<Boolean> thread = createSafeRegularExpressionMatchingThread();
        Boolean result = tryToGetResultFromThreadWithTimeout(thread);
        return result;
    }

    private CallableThread<Boolean> createSafeRegularExpressionMatchingThread() {
        final String stringToMatchForUseInThread = new String(stringToMatch);
        final String regularExpressionForUseInThread = new String(regularExpression);
        Callable<Boolean> callable = createRegularExpressionMatchingCallable(stringToMatchForUseInThread,
                regularExpressionForUseInThread);
        CallableThread<Boolean> thread = new CallableThread<Boolean>(callable);
        return thread;
    }

    private Callable<Boolean> createRegularExpressionMatchingCallable(final String stringToMatchForUseInThread,
            final String regularExpressionForUseInThread) {
        Callable<Boolean> callable = new Callable<Boolean>() {
            public Boolean call() throws Exception {
                return Boolean.valueOf(stringToMatchForUseInThread.matches(regularExpressionForUseInThread));
            }
        };
        return callable;
    }

    private Boolean tryToGetResultFromThreadWithTimeout(CallableThread<Boolean> thread) {
        startThreadAndApplyTimeout(thread);
        Boolean result = processThreadResult(thread);
        return result;
    }

    private void startThreadAndApplyTimeout(CallableThread<Boolean> thread) {
        thread.start();
        try {
            thread.join(timeoutMillis);
        } catch (InterruptedException e) {
            throwRuntimeException("Interrupt", e);
        }
    }

    private Boolean processThreadResult(CallableThread<Boolean> thread) {
        Boolean result = null;
        if (thread.isAlive()) {
            killThread(thread); // do not use anything from the thread anymore, objects may be damaged!
            throwRuntimeException("Timeout", null);
        } else {
            Exception exceptionOccurredInThread = thread.getException();
            if (exceptionOccurredInThread != null) {
                throwRuntimeException("Exception", exceptionOccurredInThread);
            } else {
                result = thread.getResult();
            }
        }
        return result;
    }

    private void throwRuntimeException(String situation, Exception e) {
        throw new RuntimeException(situation + " occured while applying pattern /" + regularExpression + "/ to input '"
                + stringToMatch + " after " + timeoutMillis + "ms!", e);
    }

    /**
     * This method uses {@link Thread#stop()} to kill a thread that is running wild. Although it is acknowledged that
     * {@link Thread#stop()} is inherently unsafe, the assumption is that the thread to kill does not hold any monitors on or
     * even references to objects referenced by the rest of the JVM, so it is acceptable to do this.
     * 
     * After calling this method nothing from the thread should be used anymore!
     * 
     * @param thread Thread to stop
     */
    @SuppressWarnings("deprecation")
    private static void killThread(CallableThread<Boolean> thread) {
        thread.stop();
    }

    private static class CallableThread<V> extends Thread {

        private final Callable<V> callable;

        private V result = null;

        private Exception exception = null;

        public CallableThread(Callable<V> callable) {
            this.callable = callable;
        }

        @Override
        public void run() {
            try {
                V result = compute();
                setResult(result);
            } catch (Exception e) {
                exception = e;
            } catch (ThreadDeath e) {
                cleanup();
            }
        }

        private V compute() throws Exception {
            return callable.call();
        }

        private synchronized void cleanup() {
            result = null;
        }

        private synchronized void setResult(V result) {
            this.result = result;
        }

        public synchronized V getResult() {
            return result;
        }

        public synchronized Exception getException() {
            return exception;
        }

    }

}

EDIT:

Thanks to dawce who pointed me to this solution I have been able to solve my original problem without the need for additional threads. I have posted the code there. Thanks to all who have responded.

Answer 1

You can use Thread.stop() if you determine its the only solution available to you. You may need to shutdown and restart your applicaton to ensure its in a good state.

Note: a Thread can capture and ignore ThreadDeath so stop isn't guarenteed to stop all threads.

An alternative way to stop a thread is to run it in a different process. This can be killed as required. This can still leave resources in an incosistent state (like lock files) but it is less likely and easier to control.

The best solution of course is to fix the code so it doesn't do this in the first place and respects Thread.interrupt() instead.

Answer 2

Instead of using Thread.stop() which is deprecated, use Thread.interrupt() which will stop raise the interrupt flag which can be checked via isInterrupted() or interrupted(), or throws an InterruptedException.

My pattern for building extending the Thread class is like this

 class MyThread extends Thread{
      private volatile boolean keepRunning = true;

      public void run(){
           while(keepRunning){
                // do my work
           }
       }

       public void killThread(){
           keepRunning = false;
           this.interrupt();
       }
 }

I'm not saying my way of handling it is perfect, there may bet better, but this works for me.

Answer 3

If the thread that needs to be stopped does not hold any (obvious) monitors on or references to objects that are used by the rest of the program, can it then be acceptable to use Thread#stop() nevertheless?

It is up to you to decide if it is "acceptable". All we can do is to advise on whether it is safe. And the answer is that it isn't.

• what about the non-obvious monitors and references that it holds?

• what about notifies, etc that it would otherwise make?

• what about actions that it might otherwise make affecting statics?

The problem is that it is difficult (in most cases) to know for sure that you've considered all of the possible interactions that the thread might have with the rest of the application.

---

Restarting the application is exactly what I try to avoid ...

It strikes me that that is the real root of your problem; i.e. you've designed a program without taking account of the fact that long-running programs need to be restarted for pragmatic reasons. Especially complicated ones that have potential bugs.

Answer 4

If you specifically design your thread code to not hold locks etc., (yes, and this includes the non-explicit locks. eg. a malloc lock that may be used when changing string sizes), then stop the thread, yes. Polling an 'interrupted' flag is fine, except that it means polling an 'interrupted' flag, ie. overhead during the 99.9999% of the time it is not actually set. This can be an issue with high-performance, tight loops.

If the check can be kept out of an innermost loop and still be checked reasonably frequently, then that is indeed the best way to go.

If the flag cannot be checked often, (eg. because of a tight loop in inaccessible library code), you could set the thread priority to the lowest possible and forget it until it does eventually die.

Another bodge that is occasionally possible is to destroy the data upon which the thread is working in such a way that the library code does exit normally, causes an exception to be raised and so control bubbles out of the opaque library code or causes an 'OnError' handler to be called. If the lib. is operating on a string, splatting the string with nulls is sure to do something. Any exception will do - if you can arrange for an AV/segfault in the thread, then fine, as long as you get control back.