1

I have tournament code for students' codes. Therefore I have no control over the students' codes.

I need to implement timeout for students' code calls (they run in separate threads). So I instrumented their code and inserted right after loop and method definitions the following code:

if (Thread.interrupted()) throw new InterruptedException();

The problem is InterruptedException is checked, and therefore I have to add throws declarations to all methods, which can break overriding methods' signatures.

So I thought I could not throw InterruptedException, but an unchecked one, e.g. RuntimeException. Can I do it? Will there be some difference?

In my tournament code I start the students' code as a futrure in an ExecutorService and try to get the result using get() with timeout.

Martin Pecka
  • 2,953
  • 1
  • 31
  • 40
  • This is really a question about `RuntimeException`s right? There is nothing special about the `InterruptedException` in itself. Care to rephrase this question and ask about threading? – Gray Jan 06 '14 at 20:06
  • @Gray There is something special about `InterruptedException` - it is checked and it should be thrown when an interruptible task is interrupted. – Martin Pecka Jan 06 '14 at 23:35
  • It is not a common pattern to have user code throw `InterruptedException`. I've added an answer @peci1. – Gray Jan 07 '14 at 13:30

4 Answers4

1

InterruptedException is suggested by java spec: you can find more information here

http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html

venergiac
  • 7,469
  • 2
  • 48
  • 70
  • That's quite informative article, thanks. Among other things, it states that "The standard thread pool (ThreadPoolExecutor) worker thread implementation is responsive to interruption, so interrupting a task running in a thread pool may have the effect of both canceling the task and notifying the execution thread that the thread pool is shutting down. If the task were to swallow the interrupt request, the worker thread might not learn that an interrupt was requested". So I think throwing a RuntimeException can cause some mess when using ThreadPoolExecutor. – Martin Pecka Jan 06 '14 at 23:33
1

If you want a simple timeout and do not care about the code terminating gracefully you could use Thread.stop(). The use of stop() is discourage because it throws a ThreadDeathException in that Thread regardless what its currently doing. By injecting code that throws an Exception in every loop you are doing essentially the same (aside from jdk code terminating normaly). So in that special case I would suggest using stop().

But if you rather want to throw the checked InterruptedException unchecked, there is a hack for that as well.

here is an example of how to stop Thread using a ScheduledExecutorService

void runWithHardTimeOut(final Runnable task, long timeout, TimeUnit unit) {

    final Thread[] runner = new Thread[1];
    final AtomicBoolean done = new AtomicBoolean(false);

    Runnable killer = new Runnable() {

        @Override
        public void run() {
            if (!done.get()) {
                while (runner[0] == null);
                if (done.get()) {
                    return;
                } else {
                    runner[0].stop();
                }
            }
        }
    };
    final Runnable r = new Runnable() {

        @Override
        public void run() {
            try {
                runner[0] = Thread.currentThread();
                killerExecutor.schedule(killer, timeout, unit);
                task.run();
                done.set(true);
            } catch (ThreadDeath wrench) {}
        }
    };

    executor.execute(r);
}
Community
  • 1
  • 1
Blank Chisui
  • 1,043
  • 10
  • 25
  • Thread.stop() is one of the options I considered. However, it is not compatible with ExecutorService which rather interrupts the thread on timeout. – Martin Pecka Jan 06 '14 at 21:24
  • In my opinion, writing a custom ExecutorService or other structure to manage timeouts that uses `stop()` is much easier than to programmatically insert interrupted checks in unknown code. – Blank Chisui Jan 06 '14 at 22:16
  • That seems to be pretty tricky, since the timeouts are handled by `FutureTask#Sync` which is an `AbstractQueuedSynchronizer` and the timeout is implemented by its `innerGet()` method, which itself sounds like something you wouldn't like to cope with. Or do you have an idea where to start? – Martin Pecka Jan 07 '14 at 00:08
  • As for the hack - I think I can always wrap the `InterruptedException` into a `RuntimeException` and setup a `try` block in the `Callable#get()` implementation, that would unwrap the inner exception if it is an `InterruptedException`. – Martin Pecka Jan 07 '14 at 00:21
  • The code example you provided looks great! I would change one thing, though - execute the killer threads in a separate executor service. That's because if you fix the number of threads in pool and all threads are consumed by long-running tasks, the killer threads get IMHO never executed. Thanks very much! – Martin Pecka Jan 07 '14 at 07:54
  • Another flaw I see is the killer thread should get scheduled only after the main computing thread starts its execution (in order not to kill it prematurely only because it didn't have a free thread to work in) – Martin Pecka Jan 07 '14 at 11:15
  • This is just a quick and dirty example outlining what I had in mind. I would suggest you modify it for your needs. – Blank Chisui Jan 07 '14 at 13:22
  • I see. I'd very much like to give you the points for correct answer, because you directed me in the right way. Unfortunately for you, your answer doesn't answer my question (but it solves my problem), whereas @erickson's answer seems to be directly answering my question. I'm just waiting for him to answer my additional question to flag his answer as accepted. – Martin Pecka Jan 07 '14 at 13:38
  • If you want to do something crazy like throwing an undeclared checked exception, at least do it with style: `Thread.currentThread().stop(new InterruptedException())` – erickson Jan 07 '14 at 17:06
1

The interrupt status of a worker thread in the standard ExecutorService implementations will be cleared when a task is completed, and it's ignored for the purposes of testing whether a task was canceled before completion (as detected by Future.isCancelled()).

So, you can safely throw an unchecked ("runtime") exception rather than InterruptedException, and it won't interfere with the operation of the ExecutorService.

However, if you are still concerned, you can preserve the interrupt status by re-asserting it:

if (Thread.interrupted()) {
  Thread.currentThread().interrupt();
  throw new IllegalStateException("Time limit exceeded.");
}

Or using a different method that doesn't clear it:

if (Thread.currentThread().isInterrupted()) 
  throw new IllegalStateException("Time limit exceeded.");

Normally custom runtime exceptions are a bad idea, because they can result in leaky abstractions, but here, a custom type in place of IllegalStateException may make it cleaner for you to distinguish your timeout exceptions from runtime exceptions raised by the student's code.


Update: Canceling a task running with ExecutorService is only reliable if the task is written correctly to support interruption, and you can't trust students to do that.

So your options are:

  1. Manually review the source code, inserting interruption detection where necessary.
  2. Get rid of ExecutorService and create worker threads yourself. Use the stop() or stop(Throwable) method to terminate the threads after timeout. To be truly robust, use a byte code engineering library like ASM to automate analysis of the compiled plugin code to see if it can catch the Throwable (or ThreadDeath or the custom Throwable) you are using to stop() the worker thread.
  3. Fork Java processes to run the agent code, and kill the whole process after timeout.
erickson
  • 265,237
  • 58
  • 395
  • 493
  • What exactly means 'when a task is completed'? You mean when it stops executing by either finish of work or an exception? – Martin Pecka Jan 07 '14 at 08:00
  • 1
    @peci1 A task is "complete" when it throws an exception, returns normally, or is canceled before starting (i.e., `isDone()` is `true`). But if the task returns (with or without exception) before it's canceled, `isCancelled()` returns `false`. Otherwise it will return `true` and `get()` will throw `CancellationException`. The interrupt status of the worker thread is cleared sometime after the `run()` or `call()` method returns, but I don't think interrupt status is used by the `ExecutorService`; it's cleared as cleanup just to make sure it doesn't affect the next task run by that thread. – erickson Jan 07 '14 at 17:00
  • @peci1 See the update to my answer about the reliability of canceling tasks. – erickson Jan 07 '14 at 17:13
  • It follows from my discussion with @Gray that stopping an ExecutorService thread using the stop() method can never be 100% safe, since the thread can be stopped right when it reports it's done its work. So back to my original solution :) Instead of manual editing as you suggest in 1), I'll stay with my "instrumentation" code, but now I see throwing InterruptedException makes no sense. Instead I have to throw a runtime exception (possibly a custom one or TimeoutException in my scenario). – Martin Pecka Jan 08 '14 at 07:57
1

I need to implement timeout for students' code calls (they run in separate threads). So I instrumented their code and inserted right after loop and method definitions the following code:
if (Thread.interrupted()) throw new InterruptedException();

It is not common for user code to throw InterruptedException. For example, I've written a ton of thread code that uses interrupts but I've never thrown InterruptedException myself. I'll quit the thread, return false, or throw some other exception if a task is interrupted instead.

Since you don't have control over the students' code then I think the best mechanism would be to manage your own threads and use thread.stop(). The reason why stop() is deprecated is that any objects that are current locked and being modified will be in an unknown state when the thread is killed. However, if the students' code is not modifying any shared state then this is not an issue.

Your code could do something like the following:

  • I would wrap the code in some sort of wrapping Runnable so you know when each code starts and when it ends. You could also catch ThreadDeath to know when thread.stop() had to be called.
  • Run each of the wrapped students' code in a thread.
  • Then join with each thread in turn with a timeout. If the timeout expires then call thread.stop().

Something like:

public class WrappedRunnable implements Runnable {
    private Runnable delegate;
    long startTimeMillis;
    long endTimeMillis;
    boolean threadDeath;
    public WrappedRunnable(Runnable delegate) {
       this.delegate = delegate;
    }
    public void run() {
        startTimeMillis = System.currentTimeMillis();
        try {
            delegate.run();
        } catch (ThreadDeath td) {
            threadDeath = true;
        }
        endTimeMillis = System.currentTimeMillis();
    }
}
Gray
  • 115,027
  • 24
  • 293
  • 354
  • Well, maybe it is not stated clearly, but I cannot do any clever modifications of the students' code, because the whole tournament has to run automatically on the submitted codes. So adding a loop as you suggest is just impossible (because the code can get stuck somwhere inside, and that's why I add an interrupted check into every loop and method call). What I need is allow a code with potential infinite loops to be safely stopped using an interrupt. So the interrupted code should stop executing and shutdown without any cleanup. That's why I wanted to throw InterruptedException. – Martin Pecka Jan 07 '14 at 13:32
  • AHA! That's the information I wasn't understanding. Sorry. Makes sense now @peci1. One thing to do is to insist that the students make some sort of callback every time through their loops although even then there is no guarantee that the code won't have bugs and kick into an infinite loop. Seems like forking your own threads and calling `thread.stop()` is maybe the only solution. `thread.stop()` is deprecated because it leaves the synchronized objects in an unknown state but if the students' code is not sharing objects with other threads then this is less of a worry. – Gray Jan 07 '14 at 13:38
  • Exactly, that's why I like @Blank Chisui's example code. No, I surely cannot trust the students that they call something in every loop :) But Thread.stop() is not the only solution - as I wrote, I can edit the students' sources before compiling in an automated way. Putting an interrupted check throwing a runtime exception into every loop and method seems as the 'clean' (but rather tricky) way to shutdown that code. – Martin Pecka Jan 07 '14 at 13:42
  • I've added some info about wrapping their runnables and catching the results of `thread.stop()` @peci1. – Gray Jan 07 '14 at 13:56
  • I assume the join has to be done from another (killer) thread in order to not block the main (controller) thread, am I right? That seems as a nice solution. But using an `ExecutorService` does most of this for me, doesn't it? – Martin Pecka Jan 07 '14 at 14:35
  • But a `ExecutorService` threads cannot be stopped. It would be a tremendously bad idea to call `stop()` on one of them. It could easily corrupt the internal service structures since they _do_ share data between themselves. – Gray Jan 07 '14 at 14:40
  • In terms of which thread does the killing, I don't know about your system. I'd have the thread which is waiting for the jobs to finish to the killing I guess @peci1. – Gray Jan 07 '14 at 14:40
  • Aha, that's possibly an important point. Are you sure `ExecutorService` doesn't cope with sudden thread death? I'd expect it to handle these situations well. In fact, calling `Thread.currentThread().stop()` just raises a `ThreadDeathException` as the next instruction, doesn't it? So stopping a thread seems to be similar to a thread throwing a normal exception. – Martin Pecka Jan 07 '14 at 15:01
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/44713/discussion-between-gray-and-peci1) – Gray Jan 07 '14 at 15:04