Replaced CircuitBreaker.allowsExecution with permitting

Presently CircuitBreaker tries to track how many executions are in progress. Standalone executions use two methods for this: allowsExecution followed by preExecute. This commit reverses the permitting process, so rather than tracking how many executions are in progress we simply track how many executions are permitted, and provide a single method to acquire a permit if possible and return whether execution is permitted.

Author: jhalterman

CHANGELOG.md

@@ -16,9 +16,9 @@ This release introduces breaking changes to the API:
 #### Policies
 
 - All policies are now threadsafe and use a builder API. The configuration methods available in the builder are mostly the same as previously with the 2.x policies. Some notes:
-  - A policy builder can be created via `.builder()`.
-  - `RetryPolicy` and `CircuitBreaker` can be constructed with default values using `.ofDefaults()`.
-  - Policy configuration is accessible via a `.getConfig()`.
+  - A policy builder can be created via `builder()`.
+  - `RetryPolicy` and `CircuitBreaker` can be constructed with default values using `ofDefaults()`.
+  - Policy configuration is accessible via a `getConfig()`.
   - Policies that have required arguments, such as `Fallback` and `Timeout`, have additional factory methods for creating a policy without using a builder, ex: `Fallback.of(this::connectToBackup)` and `Timeout.of(Duration.ofSeconds(10))`. Optional arguments must be specified through a builder, ex: `Timeout.builder(duration).withInterrupt().build()`
 
 #### Execution and AsyncExecution
@@ -40,6 +40,10 @@ This release introduces breaking changes to the API:
   - `withDelayWhen` has been renamed to `withDelayFnWhen`.
   - The above method signatures have also been changed to accept a `ContextualSupplier` instead of a `DelayFunction`, since it provides access to the same information.
 
+#### CircuitBreaker
+
+- `allowsExecution()` was removed in favor of `acquirePermit()` and `tryAcquirePermit()`, which are meant to be used with standalone CircuitBreaker usage.
+
 ### SPI Changes
 
 The following changes effect the SPI classes, for users who are extending Failsafe with custom schedulers or policies:

src/main/java/dev/failsafe/CircuitBreaker.java

@@ -92,15 +92,33 @@ enum State {
   CircuitBreakerConfig<R> getConfig();
 
   /**
-   * Returns whether the circuit allows execution and triggers a state transition if a threshold has been exceeded.
+   * Attempts to acquire a permit for the circuit breaker and throws {@link CircuitBreakerOpenException} if a permit
+   * could not be acquired.
+   *
+   * @throws CircuitBreakerOpenException
    */
-  boolean allowsExecution();
+  void acquirePermit();
+
+  /**
+   * Attempts to acquire a permit to use the circuit breaker and returns whether a permit was acquired.
+   */
+  boolean tryAcquirePermit();
+
+  /**
+   * Opens the circuit.
+   */
+  void open();
 
   /**
    * Closes the circuit.
    */
   void close();
 
+  /**
+   * Half-opens the circuit.
+   */
+  void halfOpen();
+
   /**
    * Gets the state of the circuit.
    */
@@ -162,11 +180,6 @@ enum State {
    */
   int getSuccessRate();
 
-  /**
-   * Half-opens the circuit.
-   */
-  void halfOpen();
-
   /**
    * Returns whether the circuit is closed.
    */
@@ -182,17 +195,6 @@ enum State {
    */
   boolean isOpen();
 
-  /**
-   * Opens the circuit.
-   */
-  void open();
-
-  /**
-   * Records an execution that is about to take place by incrementing the internal executions count. Required for
-   * standalone CircuitBreaker usage.
-   */
-  void preExecute();
-
   /**
    * Records an execution failure.
    */

src/main/java/dev/failsafe/internal/CircuitBreakerExecutor.java

@@ -37,11 +37,9 @@ public CircuitBreakerExecutor(CircuitBreakerImpl<R> circuitBreaker, int policyIn
 
   @Override
   protected ExecutionResult<R> preExecute() {
-    if (circuitBreaker.allowsExecution()) {
-      circuitBreaker.preExecute();
-      return null;
-    }
-    return ExecutionResult.failure(new CircuitBreakerOpenException(circuitBreaker));
+    return circuitBreaker.tryAcquirePermit() ?
+      null :
+      ExecutionResult.failure(new CircuitBreakerOpenException(circuitBreaker));
   }
 
   @Override

src/main/java/dev/failsafe/internal/CircuitBreakerImpl.java

@@ -16,15 +16,13 @@
 package dev.failsafe.internal;
 
 import dev.failsafe.*;
+import dev.failsafe.event.CircuitBreakerStateChangedEvent;
 import dev.failsafe.event.EventListener;
 import dev.failsafe.spi.DelayablePolicy;
 import dev.failsafe.spi.FailurePolicy;
-import dev.failsafe.*;
-import dev.failsafe.event.CircuitBreakerStateChangedEvent;
 import dev.failsafe.spi.PolicyExecutor;
 
 import java.time.Duration;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 /**
@@ -40,7 +38,6 @@ public class CircuitBreakerImpl<R> implements CircuitBreaker<R>, FailurePolicy<R
 
   /** Writes guarded by "this" */
   protected final AtomicReference<CircuitState<R>> state = new AtomicReference<>();
-  protected final AtomicInteger currentExecutions = new AtomicInteger();
 
   public CircuitBreakerImpl(CircuitBreakerConfig<R> config) {
     this.config = config;
@@ -53,8 +50,13 @@ public CircuitBreakerConfig<R> getConfig() {
   }
 
   @Override
-  public boolean allowsExecution() {
-    return state.get().allowsExecution();
+  public boolean tryAcquirePermit() {
+    return state.get().tryAcquirePermit();
+  }
+
+  @Override
+  public void acquirePermit() {
+    state.get().acquirePermit();
   }
 
   @Override
@@ -122,11 +124,6 @@ public void open() {
     transitionTo(State.OPEN, config.getOpenListener(), null);
   }
 
-  @Override
-  public void preExecute() {
-    currentExecutions.incrementAndGet();
-  }
-
   @Override
   public void recordFailure() {
     recordExecutionFailure(null);
@@ -144,11 +141,7 @@ public void recordResult(R result) {
 
   @Override
   public void recordSuccess() {
-    try {
-      state.get().recordSuccess();
-    } finally {
-      currentExecutions.decrementAndGet();
-    }
+    state.get().recordSuccess();
   }
 
   @Override
@@ -157,14 +150,10 @@ public String toString() {
   }
 
   protected void recordResult(R result, Throwable failure) {
-    try {
-      if (isFailure(result, failure))
-        state.get().recordFailure(null);
-      else
-        state.get().recordSuccess();
-    } finally {
-      currentExecutions.decrementAndGet();
-    }
+    if (isFailure(result, failure))
+      state.get().recordFailure(null);
+    else
+      state.get().recordSuccess();
   }
 
   /**
@@ -206,11 +195,7 @@ protected void transitionTo(State newState, EventListener<CircuitBreakerStateCha
    * Records an execution failure.
    */
   protected void recordExecutionFailure(ExecutionContext<R> context) {
-    try {
-      state.get().recordFailure(context);
-    } finally {
-      currentExecutions.decrementAndGet();
-    }
+    state.get().recordFailure(context);
   }
 
   /**

src/main/java/dev/failsafe/internal/CircuitState.java

@@ -17,6 +17,7 @@
 
 import dev.failsafe.CircuitBreaker.State;
 import dev.failsafe.CircuitBreakerConfig;
+import dev.failsafe.CircuitBreakerOpenException;
 import dev.failsafe.ExecutionContext;
 
 import java.time.Duration;
@@ -38,8 +39,6 @@ public abstract class CircuitState<R> {
     this.stats = stats;
   }
 
-  public abstract boolean allowsExecution();
-
   public Duration getRemainingDelay() {
     return Duration.ZERO;
   }
@@ -53,16 +52,28 @@ public CircuitStats getStats() {
   public synchronized void recordFailure(ExecutionContext<R> context) {
     stats.recordFailure();
     checkThreshold(context);
+    releasePermit();
   }
 
   public synchronized void recordSuccess() {
     stats.recordSuccess();
     checkThreshold(null);
+    releasePermit();
   }
 
   public void handleConfigChange() {
   }
 
   void checkThreshold(ExecutionContext<R> context) {
   }
+
+  void acquirePermit() {
+    if (!tryAcquirePermit())
+      throw new CircuitBreakerOpenException(breaker);
+  }
+
+  abstract boolean tryAcquirePermit();
+
+  void releasePermit() {
+  }
 }

src/main/java/dev/failsafe/internal/ClosedState.java

@@ -25,7 +25,7 @@ public ClosedState(CircuitBreakerImpl<R> breaker) {
   }
 
   @Override
-  public boolean allowsExecution() {
+  public boolean tryAcquirePermit() {
     return true;
   }
 

src/main/java/dev/failsafe/internal/HalfOpenState.java

@@ -19,17 +19,24 @@
 import dev.failsafe.CircuitBreaker.State;
 import dev.failsafe.ExecutionContext;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 public class HalfOpenState<R> extends CircuitState<R> {
+  protected final AtomicInteger permittedExecutions = new AtomicInteger();
+
   public HalfOpenState(CircuitBreakerImpl<R> breaker) {
     super(breaker, CircuitStats.create(breaker, capacityFor(breaker), false, null));
+    permittedExecutions.set(capacityFor(breaker));
+  }
+
+  @Override
+  public boolean tryAcquirePermit() {
+    return permittedExecutions.getAndUpdate(permits -> permits == 0 ? 0 : permits - 1) > 0;
   }
 
-  /**
-   * Ensures that the current executions are less than the thresholding capacity.
-   */
   @Override
-  public boolean allowsExecution() {
-    return breaker.currentExecutions.get() < capacityFor(breaker);
+  public void releasePermit() {
+    permittedExecutions.incrementAndGet();
   }
 
   @Override

src/main/java/dev/failsafe/internal/OpenState.java

@@ -29,10 +29,10 @@ public OpenState(CircuitBreakerImpl<R> breaker, CircuitState<R> previousState, D
   }
 
   @Override
-  public boolean allowsExecution() {
+  public boolean tryAcquirePermit() {
     if (System.nanoTime() - startTime >= delayNanos) {
       breaker.halfOpen();
-      return true;
+      return breaker.tryAcquirePermit();
     }
 
     return false;

src/test/java/dev/failsafe/CircuitBreakerTest.java

@@ -26,7 +26,7 @@
 @Test
 public class CircuitBreakerTest {
   public void shouldRequireValidDelay() {
-    assertThrows(() -> CircuitBreaker.builder().withDelay((Duration) null).build(), NullPointerException.class);
+    assertThrows(() -> CircuitBreaker.builder().withDelay(null).build(), NullPointerException.class);
     assertThrows(() -> CircuitBreaker.builder().withDelay(Duration.ofMillis(-1)).build(),
       IllegalArgumentException.class);
   }
@@ -55,7 +55,6 @@ public void shouldDefaultDelay() throws Throwable {
     CircuitBreaker<Object> breaker = CircuitBreaker.ofDefaults();
     breaker.recordFailure();
     Thread.sleep(100);
-    breaker.allowsExecution();
     assertTrue(breaker.isOpen());
   }
 

src/test/java/dev/failsafe/internal/OpenStateTest.java

@@ -25,21 +25,21 @@
 
 @Test
 public class OpenStateTest {
-  public void testAllowsExecution() throws Throwable {
+  public void testTryAcquirePermit() throws Throwable {
     // Given
     CircuitBreakerImpl<Object> breaker = (CircuitBreakerImpl<Object>) CircuitBreaker.builder()
       .withDelay(Duration.ofMillis(100))
       .build();
     breaker.open();
     OpenState<Object> state = new OpenState<>(breaker, new ClosedState<>(breaker), breaker.getConfig().getDelay());
     assertTrue(breaker.isOpen());
-    assertFalse(state.allowsExecution());
+    assertFalse(state.tryAcquirePermit());
 
     // When
     Thread.sleep(110);
 
     // Then
-    assertTrue(state.allowsExecution());
+    assertTrue(state.tryAcquirePermit());
     assertEquals(breaker.getState(), State.HALF_OPEN);
   }