Update ExecutionEvent.getStartTime to return Optional<Instant>.

Author: jhalterman

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ### API Changes
 
-- `ExecutionContext.getStartTime` now returns an `Instant` rather than a `Duration` object.
+- `ExecutionContext.getStartTime` now returns a `Instant` rather than `Duration`, and `ExecutionEvent.getStartTime` now returns `Optional<Instant>`.
 - `getFailure`, `getLastFailure`, `recordFailure` and similar methods for recording Exceptions, which were previously deprecated, were removed. Use `getException`, `getLastException`, `recordException`, etc. instead.
 
 # 3.2.4

core/src/main/java/dev/failsafe/ExecutionContext.java

@@ -73,7 +73,7 @@ public interface ExecutionContext<R> {
   R getLastResult(R defaultValue);
 
   /**
-   * Returns the time that the initial execution started, else {code null} if an execution has not started yet.
+   * Returns the time that the initial execution started.
    */
   Instant getStartTime();
 

core/src/main/java/dev/failsafe/event/ExecutionEvent.java

@@ -20,6 +20,7 @@
 
 import java.time.Duration;
 import java.time.Instant;
+import java.util.Optional;
 
 /**
  * Encapsulates information about a Failsafe execution.
@@ -61,8 +62,8 @@ public int getExecutionCount() {
   /**
    * Returns the time that the initial execution started, else {code null} if an execution has not started yet.
    */
-  public Instant getStartTime() {
-    return context.getStartTime();
+  public Optional<Instant> getStartTime() {
+    return Optional.ofNullable(context.getStartTime());
   }
 
   /**

core/src/test/java/dev/failsafe/functional/BlockedExecutionTest.java

@@ -18,6 +18,7 @@
 import dev.failsafe.*;
 import dev.failsafe.testing.Asserts;
 import dev.failsafe.testing.Testing;
+import net.jodah.concurrentunit.Waiter;
 import org.testng.annotations.Test;
 
 import java.time.Duration;
@@ -30,7 +31,7 @@
  * Tests scenarios against a small threadpool where executions could be temporarily blocked.
  */
 @Test
-public class BlockedExecutionTest {
+public class BlockedExecutionTest extends Testing {
   /**
    * Asserts that a scheduled execution that is blocked on a threadpool is properly cancelled when a timeout occurs.
    */
@@ -49,6 +50,7 @@ public void shouldCancelScheduledExecutionOnTimeout() throws Throwable {
       TimeoutExceededException.class);
     Thread.sleep(300);
     assertFalse(supplierCalled.get());
+    executor.shutdownNow();
   }
 
   /**
@@ -70,6 +72,7 @@ public void shouldCancelScheduledRetryOnTimeout() {
 
     Asserts.assertThrows(() -> future.get(500, TimeUnit.MILLISECONDS), ExecutionException.class,
       TimeoutExceededException.class);
+    executor.shutdownNow();
   }
 
   /**
@@ -93,6 +96,7 @@ public void shouldCancelScheduledFallbackOnTimeout() {
     Asserts.assertThrows(() -> future.get(500, TimeUnit.MILLISECONDS), ExecutionException.class,
       TimeoutExceededException.class);
     assertFalse(fallbackCalled.get());
+    executor.shutdownNow();
   }
 
   /**
@@ -117,5 +121,31 @@ public void shouldCancelScheduledFallbackOnCancel() throws Throwable {
     Asserts.assertThrows(future::get, CancellationException.class);
     Thread.sleep(300);
     assertFalse(fallbackCalled.get());
+    executor.shutdownNow();
+  }
+
+  /**
+   * Asserts that start times are not populated in execution events for an execution that times out while blocked on a
+   * thread pool, and never starts.
+   */
+  public void shouldNotPopulateStartTime() throws Throwable {
+    Waiter waiter = new Waiter();
+    Timeout<Object> timeout = Timeout.builder(Duration.ofMillis(50)).withInterrupt().onFailure(e -> {
+      waiter.assertTrue(!e.getStartTime().isPresent());
+    }).build();
+    ExecutorService executor = Executors.newFixedThreadPool(1);
+    executor.execute(uncheck(() -> {
+      Thread.sleep(500);
+    }));
+
+    Failsafe.with(timeout).with(executor).onComplete(e -> {
+      waiter.assertTrue(!e.getStartTime().isPresent());
+      waiter.resume();
+    }).runAsync(() -> {
+      waiter.fail("Execution should not start due to timeout");
+    });
+
+    waiter.await(1, TimeUnit.SECONDS);
+    executor.shutdownNow();
   }
 }

core/src/test/java/dev/failsafe/issues/Issue231Test.java

@@ -33,5 +33,6 @@ public void shouldWaitForExecutionCompletion() {
       }
     }).get(), ExecutionException.class, TimeoutExceededException.class);
     assertTrue(executionCompleted.get());
+    executorService.shutdownNow();
   }
 }

core/src/test/java/dev/failsafe/issues/Issue260Test.java

@@ -42,5 +42,6 @@ public void test() throws Throwable {
     f1.get(1, TimeUnit.SECONDS);
     f2.get(1, TimeUnit.SECONDS);
     f3.get(1, TimeUnit.SECONDS);
+    executor.shutdownNow();
   }
 }

core/src/test/java/dev/failsafe/issues/Issue311Test.java

@@ -7,6 +7,7 @@
 
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicInteger;
 
@@ -25,6 +26,7 @@ public void failsafeFail() throws Throwable {
       })
       .get();
     assertEquals(counter.get(), 2);
+    ((ExecutorService) executor).shutdownNow();
   }
 
   public void testNullCompletionStage() throws Throwable {