Adding a timeout to commands by wrapping + thread - suggestions?

[dduck]

Hi,

As I know this is something that is notoriously hard to get right, I
thought I would post my naiive implementation here for comments and
advice.

The problem is a common one: We have several actions that all comply
with the same interface (in this example: Command), but it turns out
that some of them may get "stuck" if e.g. a mountpoint goes missing
during execution, which may happen once in a blue moon. Therefore we
want to develop a common facility to monitor any action for excessive
time-to-complete, and either log the problem or kill the action and
report failure up the chain by an Exception.

I came up with this naiive solution:

NB: Stopwatch is a homebrew class that measures time passed since
construction in millis.

/**
* A simple command.
*/
public interface Command {
void execute();
}

/**
* A thread that runs a command.
*/
public class CommandThread extends Thread {
private Command cmd;

public CommandThread(Command cmd) {
this.cmd = cmd;
}

public void run() {
cmd.execute();
}
}

/**
* A command that sleeps for some millis.
*/
public class SleepingCmd implements Command {

private long howLongL;

public SleepingCmd(Long howLongL) {
this.howLongL = howLongL;
}

public void execute() {
try {
Thread.sleep(howLongL);
} catch (InterruptedException e) {
throw new RuntimeException("I was interrupted!");
}
}
}

public class TimeoutWrappingCmd implements Command {

private Command wrappedCmd;

private long timeoutL;

public TimeoutWrappingCmd(Command wrappedCmd, long timeoutL) {
this.wrappedCmd = wrappedCmd;
this.timeoutL = timeoutL;
}

public void execute() {
Thread cmdThread = new CommandThread(wrappedCmd);
cmdThread.start();
Stopwatch sw = new Stopwatch();
while (sw.elapsedMillisL() < timeoutL && cmdThread.isAlive())
{
try {
Thread.sleep(100L);
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting to poll
encapsulated command");
}
}
if (cmdThread.isAlive()) {
cmdThread.interrupt();
}
try {
cmdThread.join();
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated
command to join");
}
}

}

Comments?

Sincerely,
Anders S. Johansen, Royal Danish Library

 

[Lew]

Comments?

Read /Java Concurrency in Practice/ by Brian Goetz.

Among other things, it's chock-full of idioms for the type of thing you're
trying to do. It will also help you avoid some of the trouble you'll be in.
I'm certainly no expert; this book helps me to realize that.

 

[Daniel Pitts]

Read /Java Concurrency in Practice/ by Brian Goetz.

I second that. One of the best books I've ever read on any subject in
Java. Well written, easy to read, and very accurate.

Among other things, it's chock-full of idioms for the type of thing you're
trying to do. It will also help you avoid some of the trouble you'll be in.
I'm certainly no expert; this book helps me to realize that.

Also, look into the interface Callable<E>, or Runnable, it does pretty
much what your Command interface does, but its standard. Callable and
Runnable also gives you the option of using Executors, which are
useful for concurrent tasks, and managing timeouts.

Good luck,
Daniel.

 

[dduck]

Well, I have refined the example a bit, but have a new problem:
Exceptions from the thread...

If I wrap a command in a Runnable, it might throw a (runtime)
exception. Apparently such exceptions are only reported, but not
passed on to the calling thread.

I can catch them using an implementation of ThreadGroup that
implements uncaughtException, but re-throwing does not propagate to
the main thread either.

The code below outputs:
Caught exception - throwing a new one
Command was executed just fine

....where I had hoped it would instead die with an exception before
printing "Command was executed just fine".

-- code --

public class OnTimeoutExecuteCmd implements Command {

private Command wrappedCmd;

private Command onTimeoutCmd;

private long timeoutL;

public OnTimeoutExecuteCmd(Command wrappedCmd, Command
onTimeoutCmd, long timeoutL) {
this.wrappedCmd = wrappedCmd;
this.onTimeoutCmd = onTimeoutCmd;
this.timeoutL = timeoutL;
}

static class OverrideThreadGroup extends ThreadGroup {

public OverrideThreadGroup() {
super("Re-throw RTE");
}

public void uncaughtException(Thread t, Throwable e) {
System.out.println("Caught exception - throwing a new
one");
throw new RuntimeException("Uncaught exception from
thread", e);
}



}

public void execute() {
Thread cmdThread = new Thread(new
CommandRunnable(wrappedCmd));
cmdThread.setUncaughtExceptionHandler(new
OverrideThreadGroup());
cmdThread.start();
try {
cmdThread.join(timeoutL);
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated command
to "
+ "join or timeout to expire");
}
if (cmdThread.isAlive()) {
onTimeoutCmd.execute();
}
try {
cmdThread.join();
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated
command to join");
}
}

}


public class ThrowingCmd implements Command {

public void execute() {
System.out.println("Throwing now");
throw new RuntimeException("I threw in the towel");

}

}


public class TestOnTimeout {

public static void main(String[] args) {
Command ote = new OnTimeoutExecuteCmd(new ThrowingCmd(), new
HelloCmd(), 800L);
try {
ote.execute();
System.out.println("Command was executed just fine");
} catch (RuntimeException e) {
System.err.println("Runtime exception from ote");
}
}

}

 

[dduck]

Follow-up:

By changing the execute method like so:

public void execute() {
Thread cmdThread = new Thread(new
CommandRunnable(wrappedCmd));
Thread.currentThread().setUncaughtExceptionHandler(new
OverrideThreadGroup());
cmdThread.start();
try {
cmdThread.join(timeoutL);
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated command
to "
+ "join or timeout to expire");
}
if (cmdThread.isAlive()) {
onTimeoutCmd.execute();
}
try {
cmdThread.join();
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated
command to join");
}
}


....I now get the re-.thrown exception, BUT not instantly. The output
is:

Throwing now
Exception in thread "Thread-0" java.lang.RuntimeException: I threw in
the towel
at dk.kb.doms.experimental.ThrowingCmd.execute(ThrowingCmd.java:7)
at dk.kb.doms.experimental.CommandRunnable.run(CommandRunnable.java:
17)
at java.lang.Thread.run(Thread.java:613)
Command was executed just fine

I.e. the exception is thrown, but stil not in the main thread.

I guess if all I want is a log message when a command has executed
past the timout I should use a TimerTask instead?

 

[Ingo R. Homann]

Hi,

I do not really know what you expect the program to do. But if an
Exception (RuntimeException, Error or whatelse) is thrown in a Thread,
no other Thread will be informed about that (espezially not the Thread
that started the thread in which the exception is thrown). You might
indicate that by a flag or even by exposing the Exception:


class MyRunnable implements Runnable {

private Exception e;

public Exception getException() {
return e;
}

public void run() {
try {
// do something that might throw an Exception
} catch(Exception e) {
this.e=e;
}
}

}


calling it like this:


MyRunnable r=new MyRunnable();
Thread t=new Thread(r);
r.start();
r.join();
if(r.getException()) {
...
}


Of course, this violates the general contract that every Thread must
care about its own exceptions. (Note that this might sometimes be useful
e.g. in Server-Threads that do not have an own GUI.)

Ciao,
Ingo

 

[Roedy Green]

The problem is a common one: We have several actions that all comply
with the same interface (in this example: Command), but it turns out
that some of them may get "stuck" if e.g. a mountpoint goes missing
during execution, which may happen once in a blue moon. Therefore we
want to develop a common facility to monitor any action for excessive
time-to-complete, and either log the problem or kill the action and
report failure up the chain by an Exception.

check out the new java.util.concurrent to see if there is a suitable
tool in there. See http://mindprod.com/jgloss/queue.html to get you
started.

If there is not, a Timer would be easier that doing it by hand with a
separate thread. Just have it go off every x seconds, wake up, poke
around to see if all is behaving. See
http://mindprod.com/jgloss/timer.html

Killing a misbehaving thread is deprecated. You are supposed to just
ask it gently to commit suicide. However, if it has gone of the
rails, it won't like pay any attention to you.

 

[Lew]

Follow-up: ....
I.e. the exception is thrown, but stil not in the main thread.

Read /Java Concurrency in Practice/ by Brian Goetz.

It talks about how to deal with exceptions that a thread throws, so that an
invoking thread can deal with them.

 

[Daniel Pitts]

Follow-up:

By changing the execute method like so:

public void execute() {
Thread cmdThread = new Thread(new
CommandRunnable(wrappedCmd));
Thread.currentThread().setUncaughtExceptionHandler(new
OverrideThreadGroup());
cmdThread.start();
try {
cmdThread.join(timeoutL);
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated command
to "
+ "join or timeout to expire");
}
if (cmdThread.isAlive()) {
onTimeoutCmd.execute();
}
try {
cmdThread.join();
} catch (InterruptedException e) {
throw new RuntimeException(
"I was interrupted while waiting for encapsulated
command to join");
}
}

...I now get the re-.thrown exception, BUT not instantly. The output
is:

Throwing now
Exception in thread "Thread-0" java.lang.RuntimeException: I threw in
the towel
at dk.kb.doms.experimental.ThrowingCmd.execute(ThrowingCmd.java:7)
at dk.kb.doms.experimental.CommandRunnable.run(CommandRunnable.java:
17)
at java.lang.Thread.run(Thread.java:613)
Command was executed just fine

I.e. the exception is thrown, but stil not in the main thread.

I guess if all I want is a log message when a command has executed
past the timout I should use a TimerTask instead?

Take a look at the Executor class and the java.util.concurrency
packages.

An Executor will return a Future object, which you can get the return
value of your work unit, or the exception that it throws.

Alternatively, you can wrap your whole run method in a try {} catch
(Throwable t) { }, and save the throwable for your main thread to
handle after the fact. I would *not* suggest this approach.

 

[Ingo R. Homann]

Hi,

note that my code was only an example to show the concept. In real world
applications, of course, you might encapsulate it like this (written in
my newsreader, so untested):


class MyProcess // no not implement Runnable!
{

private Exception e;

private Thread t;

public void start() {
t=new Thread(new Runnable(){
public void run() {
try {
// do something that might throw an Exception
} catch(Exception e) {
this.e=e;
}
});
t.start();
}

public void join throws Exception {
t.join();
if(e!=null) {
throw e;
}
}

}


Note that this is also only a "proof of concept" and there are many
things missing (such as catching the problem of joining an unstarted
Thread). You may also add many methods like isRunning, interrupt and so on.

Ciao,
Ingo

 

[Lew]

Hi,

note that my code was only an example to show the concept. In real world
applications, of course, you might encapsulate it like this (written in
my newsreader, so untested):


class MyProcess // no not implement Runnable!
{

private Exception e;

private Thread t;

public void start() {
t=new Thread(new Runnable(){
public void run() {
try {
// do something that might throw an Exception
} catch(Exception e) {
this.e=e;
}
});
t.start();
}

public void join throws Exception {
t.join();
if(e!=null) {
throw e;
}
}

}


Note that this is also only a "proof of concept" and there are many
things missing (such as catching the problem of joining an unstarted
Thread). You may also add many methods like isRunning, interrupt and so on.

Better make 'e' volatile!

 

[Lew]

Hi,

note that my code was only an example to show the concept. In real world
applications, of course, you might encapsulate it like this (written in
my newsreader, so untested):


class MyProcess // no not implement Runnable!
{

private Exception e;

private Thread t;

public void start() {
t=new Thread(new Runnable(){
public void run() {
try {
// do something that might throw an Exception
} catch(Exception e) {
this.e=e;
}
});
t.start();
}

public void join throws Exception {
t.join();
if(e!=null) {
throw e;
}
}

}


Note that this is also only a "proof of concept" and there are many
things missing (such as catching the problem of joining an unstarted
Thread). You may also add many methods like isRunning, interrupt and so on.

For a second I though 'e' needed to be volatile, then I remembered that
Thread.start() and join() handle the synchronization and memory model visibility.