Retry Class in Java

Question

I have the following Java code used to retry a certain actions/methods

package helpers;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.*;
public final class Retry {
 public static <V> V execute(Callable<V> action, Duration retryInterval, int retryCount)
 throws AggregateException {
 List<Throwable> exceptions = new ArrayList<>();
 for (int retry = 0; retry < retryCount; ++retry) {
 try {
 if (retry > 0)
 Thread.sleep(retryInterval.toMillis());
 return action.call();
 } catch (Throwable t) {
 exceptions.add(t);
 }
 }
 throw new AggregateException(exceptions);
 }
 public static <V> Future executeAsync(Callable<V> action, Duration retryInterval, int retryCount)
 throws AggregateException {
 FutureTask<V> task = new FutureTask<>(action);
 ExecutorService executor = Executors.newSingleThreadExecutor();
 return executor.submit(task);
 }
}

Q1. Can this code be improved?

Q2. Am I using the Future interface correctly in the asynchronous version of execute?

I am also attempting to test this class using

package helpers;
import org.junit.Before;
import org.junit.Test;
import java.text.MessageFormat;
import java.time.Duration;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import static org.junit.Assert.assertEquals;
public class RetryTest {
 private Integer counter = 0;
 private Callable<Integer> calculator = null;
 @Before
 public void init() {
 calculator = () -> {
 for (int i = 0; i < 3; ++i) {
 counter++;
 System.out.println(MessageFormat.format(
 "Counter = {0}", Integer.toString(counter)));
 }
 if (counter < 9)
 throw new Exception();
 return counter;
 };
 }
 @Test
 public void execute() throws AggregateException {
 Integer result = Retry.execute(calculator, Duration.ofMillis(50), 9);
 assertEquals(9, (int)result);
 }
 @Test 
 public void executeAsync() throws AggregateException, ExecutionException, InterruptedException {
 Future future = Retry.executeAsync(calculator, Duration.ofMillis(500), 9);
 while (!future.isDone()) {
 System.out.println("Churn");
 }
 assertEquals(9, (int)future.get());
 }
}

Q3. Is this the correct way to test that my async version is actually async?

• mtj: There must be a flaw in your test, as your async variant does not perform any retry action thus failing with an exception and no result. Try to reset the counter before each test and you'll see this.

Answer 1

Some thoughts about your implementation:

As stated by @mtj, executeAsync() doesn’t actually retry.

Don’t ever catch Throwable. You do want to catch Exception, but Throwable is things like OutOfMemoryError. You’re not able to handle those, and shouldn’t try.

You should separately handle an InterruptedException. Right now that just goes into your AggregateException and gets ignored. You should actually interrupt what you’re doing and return if an InterruptedException gets thrown from sleep().

retry is not a great variable name. Perhaps attempt would be better?

As a general design point, I think you’d be better served with a wrapper class than a static method. That would let you use the rest of the concurrent API, such as executors, etc. Such a class might look something like:

import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
public final class RetryingCallable<V> implements Callable<V> {
 private final Callable<V> callable;
 private final Duration retryInterval;
 private final int retryCount;
 public RetryingCallable(final Callable<V> callable, final Duration retryInterval, final int retryCount) {
 this.callable = callable;
 this.retryInterval = retryInterval;
 this.retryCount = retryCount;
 }
 @Override
 public V call() throws Exception {
 final List<Exception> exceptions = new ArrayList<>();
 for (int attempt = 0; attempt < this.retryCount; attempt++) {
 if (attempt > 0) {
 this.sleep(exceptions);
 }
 try {
 return this.callable.call();
 } catch (final Exception e) {
 exceptions.add(e);
 }
 }
 throw new AggregateException(exceptions);
 }
 private void sleep(final List<Exception> exceptions) throws AggregateException {
 try {
 Thread.sleep(this.retryInterval.toMillis());
 } catch (final InterruptedException e) {
 exceptions.add(e);
 Thread.currentThread().interrupt();
 throw new AggregateException(exceptions);
 }
 }
}