3

I have following code in java-

@Override
public void run() {

    logger.info("Thread Started:" + this.getName());

    try {
        runJob();
    } catch( Throwable e) {
        logger.error("Exception in running thread: " + this.getName() + ", restarting job", e);
        run();
    }
}

public void runJob() {

    while(true) {
       try {
           // Work here
       } catch(Exception e) {
           // log the exception
       }
    }
 }

Is this code actually be going to keep the thread alive in every case and is this only the way to recover thread?

This is the alternative I thought of after reading all the answers. Let me know if this is a good way for keeping the thread alive forever even if Error occurred:

@Override
public void run() {

    logger.info("Thread Started:" + this.getName());

    while(true) {
      try {
        runJob();
      } catch( Throwable e) {
        logger.error("Exception in running thread: " + this.getName() + ", restarting job", e);
        }
    }
}

public void runJob() {

     try {
          // Work here
     } catch(Exception e) {
          // log the exception
     }
 }
Nikolas Charalambidis
  • 40,893
  • 16
  • 117
  • 183
thedevd
  • 683
  • 11
  • 26
  • 3
    You are doing a recursive call of the function which can, depending how often you go in your `catch` clause, cause a StackOverflowError. Consider using other solutions such as a loop, etc. – Ben May 22 '18 at 09:23
  • This way you will also catch OutOfMemoryError, which you should not do. If at all then catch `Exception` for unchecked exceptions only leaving all `Error` derivatives out. Even better catch `Exception` and propagate it outside e.g. via `CompletableFuture` or some other asynchronous mechanism, so that you can define generic error handling in case of exceptions in threads. And yes, the comment about a recursive call into a failing function is most valid independently of the exception handling details. – Oleg Sklyar May 22 '18 at 09:25
  • You should monitor the thread from where you start it. Implement a watch dog to track the health of the thread. Let the watch dog also be able to either restart the stale thread or start a new one. – Korashen May 22 '18 at 09:25
  • At least, only catch `Exception` instead of `Throwable`. That way the `Error` will not be catched. Since it is unlikely that you can get back on your feet after an `Error` occurred. For the rest, I fear this will be opinion based – AxelH May 22 '18 at 09:30
  • Thanks everyone, please see the edit. – thedevd May 22 '18 at 09:59

3 Answers3

7

This has little to do with "threads". Retry logic can be implemented on exceptions, and that's common practice.

However, catching Throwable is clearly a dangerous thing to do. You would want to retry on select errors (such as timeout, failed connection, etc), which of course requires finer knowledge of how your runJob() method runs.

Beside that, your retry implementation is making a recursive call which can also be bad. You'll end up running into a stack overflow error (and also catch it with your Throwbale catch block, altogether leading to weird behavior). Rather loop and execute runJob() repeatedly.

boolean retry = false;
do {
    try {
        runJob();
        retry = false;
    } catch (SpecificException e) { //timeout, network failure exceptions
        logger.error("Exception in running thread: "
            + this.getName() + ", restarting job");
        retry = true;
    }
} while(retry);

You may also need to add a counter to limit the number of retries.

ernest_k
  • 44,416
  • 5
  • 53
  • 99
  • 3
    In your `while` loop's condition you have the assignment operator for some reason. You might want to edit that. – Ben May 22 '18 at 09:33
  • 1
    The use of the term _error_ is dangerous when talking about _"select errors (such as timeout, failed connection, etc)"_ since those are `Exception` not `Error` sublcasses. – AxelH May 22 '18 at 09:34
  • `while (retry == true)` is too verbose. I think `while (retry)` looks better. – DodgyCodeException May 22 '18 at 09:36
  • @Ben Thanks. Now I know that even an IDE warning doesn't help. It takes more than 1 dev to properly check a boolean variable :-) – ernest_k May 22 '18 at 09:42
  • 1
    @ErnestKiwele a good notation to prevent any typo with the equal operator is to use the [_yoda condition_](https://en.wikipedia.org/wiki/Yoda_conditions). `retry == true` -> `true == retry`. That way `true = retry` will not compile. Of course for boolean you can simply use `retry` but that works for any type – AxelH May 22 '18 at 09:44
  • Thanks, please see the edit to let me know. Basically I want my thread alive forever irrespective of Error + Exception. Is this possible with that alternative – thedevd May 22 '18 at 10:00
  • 1
    @thedevd It's tempting to catch Throwable, but that's more a problem than a solution. What if you run out of memory? Does it make sense to continue retrying? For some errors, it in fact makes sense to let the process die, and that's something to handle in how you manage your process. You could argue that one can log the errors to disk etc, but a disk can also get full (you get the idea: `Throwable` is not a good thing to catch). It should be enough to retry on all specific exceptions that can be expected or that can arise during tests. – ernest_k May 22 '18 at 10:09
3

You should never control the flow with try-catch!

Moreover, if you call this failed method in the catch, it will be called again causing the recursive neverending cycle, thus StackOverflowError.

I suggest you create a counter to limit a maximum number of attempts. For this use-case, get inspired at the question How do you implement a re-try-catch?

There are many libraries offering the recovery mechanism. I recommend you the jcabi-aspects.

Nikolas Charalambidis
  • 40,893
  • 16
  • 117
  • 183
  • I seems the link you have provided would be a good candidate for a duplicate. Not to answer the qusetion directly (it is asking for opinion) but to provide a correct alternative. – AxelH May 22 '18 at 09:32
3

It will not 'recover' it but rather start over and try again. This can lead to an endless loop of recursive calls until a StackOverflow occurs. And in this case your application will be stopped anyway.

Imagine accessing a file that does not exist (due to a typo or something). You would always catch a FileNotFoundException and start over attempting to read the same non-existing file again and again and again...

Starting over the same task over and over again without changing anything will almost always lead to the same exception and the above stated case happens.A proper exception-handling is always more important than to keep alive your program under any circumstances. Let the user of your software know whatthe issue is. Maybe it's something he can change. Maybe not. In any case it's mostlikely better to abort the thread instead of recursively start over.

L.Spillner
  • 1,772
  • 10
  • 19