Guard against potential race between Timeout interrupt and execution completion

Author: jhalterman

src/main/java/net/jodah/failsafe/AbstractExecution.java

@@ -35,11 +35,16 @@ public abstract class AbstractExecution extends ExecutionContext {
   final List<PolicyExecutor<Policy<Object>>> policyExecutors;
 
   // Internally mutable state
+  /* Whether a result has been post-executed */
   volatile boolean resultHandled;
-  /** The wait time in nanoseconds. */
+  /* Whether the execution can be interrupted */
+  volatile boolean canInterrupt;
+  /* Whether the execution has been interrupted */
+  volatile boolean interrupted;
+  /* The wait time in nanoseconds. */
   private volatile long waitNanos;
+  /* Whether the execution has been completed */
   volatile boolean completed;
-  volatile boolean interrupted;
 
   /**
    * Creates a new AbstractExecution for the {@code executor}.
@@ -72,9 +77,10 @@ void preExecute() {
     attemptStartTime = Duration.ofNanos(System.nanoTime());
     if (startTime == Duration.ZERO)
       startTime = attemptStartTime;
+    resultHandled = false;
     cancelled = false;
+    canInterrupt = true;
     interrupted = false;
-    resultHandled = false;
   }
 
   boolean isAsyncExecution() {

src/main/java/net/jodah/failsafe/Functions.java

@@ -285,17 +285,21 @@ static <T> Supplier<ExecutionResult> resultSupplierOf(CheckedSupplier<T> supplie
         result = ExecutionResult.success(supplier.get());
       } catch (Throwable t) {
         throwable = t;
-        // Propogate InterruptedException if not intentionally interrupted by a timeout
-        if (!execution.interrupted && t instanceof InterruptedException)
-          Thread.currentThread().interrupt();
         result = ExecutionResult.failure(t);
       } finally {
-        // Clear interrupt flag if execution was meant to be interrupted
-        if (execution.interrupted && (throwable == null || !(throwable instanceof InterruptedException)))
-          Thread.interrupted();
+        // Guard against race with Timeout interruption
+        synchronized (execution) {
+          execution.canInterrupt = false;
+          if (execution.interrupted)
+            // Clear interrupt flag if interruption was intended
+            Thread.interrupted();
+          else if (throwable instanceof InterruptedException)
+            // Set interrupt flag if interrupt occurred but was not intentional
+            Thread.currentThread().interrupt();
 
-        if (result != null)
-          execution.record(result);
+          if (result != null)
+            execution.record(result);
+        }
       }
       return result;
     };

src/main/java/net/jodah/failsafe/TimeoutExecutor.java

@@ -66,13 +66,18 @@ protected Supplier<ExecutionResult> supply(Supplier<ExecutionResult> supplier, S
           if (result.getAndUpdate(v -> v != null ? v : ExecutionResult.failure(new TimeoutExceededException(policy)))
             == null) {
             if (policy.canCancel()) {
-              boolean canInterrupt = policy.canInterrupt();
-              if (canInterrupt)
-                execution.record(result.get());
+              // Cancel and interrupt
               execution.cancelled = true;
-              execution.interrupted = canInterrupt;
-              if (canInterrupt)
-                executionThread.interrupt();
+              if (policy.canInterrupt()) {
+                // Guard against race with the execution completing
+                synchronized (execution) {
+                  if (execution.canInterrupt) {
+                    execution.record(result.get());
+                    execution.interrupted = true;
+                    executionThread.interrupt();
+                  }
+                }
+              }
             }
           }
           return null;