From 4ecc7c889cad7bfa6cb574af52252480f7e0d7cf Mon Sep 17 00:00:00 2001 From: Lakshmi Narasiman <6291010+laksnv@users.noreply.github.com> Date: Mon, 2 Mar 2020 01:30:15 -0800 Subject: [PATCH] Issue #891: Make Circuit Breaker state transition calls idempotent (#892) --- .../circuitbreaker/CircuitBreaker.java | 28 +++++++-- .../internal/CircuitBreakerStateMachine.java | 3 + .../CircuitBreakerStateMachineTest.java | 59 ++++++++++++++++--- 3 files changed, 75 insertions(+), 15 deletions(-) diff --git a/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/CircuitBreaker.java b/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/CircuitBreaker.java index de1a2b7fd7..d78e905284 100644 --- a/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/CircuitBreaker.java +++ b/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/CircuitBreaker.java @@ -567,7 +567,8 @@ static Supplier> decorateFuture(CircuitBreaker circuitBreaker, void reset(); /** - * Transitions the state machine to CLOSED state. + * Transitions the state machine to CLOSED state. This call is idempotent and will not have + * any effect if the state machine is already in CLOSED state. *

* Should only be used, when you want to force a state transition. State transition are normally * done internally. @@ -575,7 +576,8 @@ static Supplier> decorateFuture(CircuitBreaker circuitBreaker, void transitionToClosedState(); /** - * Transitions the state machine to OPEN state. + * Transitions the state machine to OPEN state. This call is idempotent and will not have + * any effect if the state machine is already in OPEN state. *

* Should only be used, when you want to force a state transition. State transition are normally * done internally. @@ -583,7 +585,8 @@ static Supplier> decorateFuture(CircuitBreaker circuitBreaker, void transitionToOpenState(); /** - * Transitions the state machine to HALF_OPEN state. + * Transitions the state machine to HALF_OPEN state. This call is idempotent and will not have + * any effect if the state machine is already in HALF_OPEN state. *

* Should only be used, when you want to force a state transition. State transition are normally * done internally. @@ -592,7 +595,8 @@ static Supplier> decorateFuture(CircuitBreaker circuitBreaker, /** * Transitions the state machine to a DISABLED state, stopping state transition, metrics and - * event publishing. + * event publishing. This call is idempotent and will not have any effect if the + * state machine is already in DISABLED state. *

* Should only be used, when you want to disable the circuit breaker allowing all calls to pass. * To recover from this state you must force a new state transition @@ -601,7 +605,8 @@ static Supplier> decorateFuture(CircuitBreaker circuitBreaker, /** * Transitions the state machine to METRICS_ONLY state, stopping all state transitions but - * continues to capture metrics and publish events. + * continues to capture metrics and publish events. This call is idempotent and will not have + * any effect if the state machine is already in METRICS_ONLY state. *

* Should only be used when you want to collect metrics while keeping the circuit breaker * disabled, allowing all calls to pass. @@ -611,7 +616,8 @@ static Supplier> decorateFuture(CircuitBreaker circuitBreaker, /** * Transitions the state machine to a FORCED_OPEN state, stopping state transition, metrics and - * event publishing. + * event publishing. This call is idempotent and will not have any effect if the state machine + * is already in FORCED_OPEN state. *

* Should only be used, when you want to disable the circuit breaker allowing no call to pass. * To recover from this state you must force a new state transition @@ -930,30 +936,36 @@ public int getOrder() { * State transitions of the CircuitBreaker state machine. */ enum StateTransition { + CLOSED_TO_CLOSED(State.CLOSED, State.CLOSED), CLOSED_TO_OPEN(State.CLOSED, State.OPEN), CLOSED_TO_DISABLED(State.CLOSED, State.DISABLED), CLOSED_TO_METRICS_ONLY(State.CLOSED, State.METRICS_ONLY), CLOSED_TO_FORCED_OPEN(State.CLOSED, State.FORCED_OPEN), + HALF_OPEN_TO_HALF_OPEN(State.HALF_OPEN, State.HALF_OPEN), HALF_OPEN_TO_CLOSED(State.HALF_OPEN, State.CLOSED), HALF_OPEN_TO_OPEN(State.HALF_OPEN, State.OPEN), HALF_OPEN_TO_DISABLED(State.HALF_OPEN, State.DISABLED), HALF_OPEN_TO_METRICS_ONLY(State.HALF_OPEN, State.METRICS_ONLY), HALF_OPEN_TO_FORCED_OPEN(State.HALF_OPEN, State.FORCED_OPEN), + OPEN_TO_OPEN(State.OPEN, State.OPEN), OPEN_TO_CLOSED(State.OPEN, State.CLOSED), OPEN_TO_HALF_OPEN(State.OPEN, State.HALF_OPEN), OPEN_TO_DISABLED(State.OPEN, State.DISABLED), OPEN_TO_METRICS_ONLY(State.OPEN, State.METRICS_ONLY), OPEN_TO_FORCED_OPEN(State.OPEN, State.FORCED_OPEN), + FORCED_OPEN_TO_FORCED_OPEN(State.FORCED_OPEN, State.FORCED_OPEN), FORCED_OPEN_TO_CLOSED(State.FORCED_OPEN, State.CLOSED), FORCED_OPEN_TO_OPEN(State.FORCED_OPEN, State.OPEN), FORCED_OPEN_TO_DISABLED(State.FORCED_OPEN, State.DISABLED), FORCED_OPEN_TO_METRICS_ONLY(State.FORCED_OPEN, State.METRICS_ONLY), FORCED_OPEN_TO_HALF_OPEN(State.FORCED_OPEN, State.HALF_OPEN), + DISABLED_TO_DISABLED(State.DISABLED, State.DISABLED), DISABLED_TO_CLOSED(State.DISABLED, State.CLOSED), DISABLED_TO_OPEN(State.DISABLED, State.OPEN), DISABLED_TO_FORCED_OPEN(State.DISABLED, State.FORCED_OPEN), DISABLED_TO_HALF_OPEN(State.DISABLED, State.HALF_OPEN), DISABLED_TO_METRICS_ONLY(State.DISABLED, State.METRICS_ONLY), + METRICS_ONLY_TO_METRICS_ONLY(State.METRICS_ONLY, State.METRICS_ONLY), METRICS_ONLY_TO_CLOSED(State.METRICS_ONLY, State.CLOSED), METRICS_ONLY_TO_FORCED_OPEN(State.METRICS_ONLY, State.FORCED_OPEN), METRICS_ONLY_TO_DISABLED(State.METRICS_ONLY, State.DISABLED); @@ -987,6 +999,10 @@ public State getToState() { return toState; } + public static boolean isInternalTransition(final StateTransition transition) { + return transition.getToState() == transition.getFromState(); + } + @Override public String toString() { return String.format("State transition from %s to %s", fromState, toState); diff --git a/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachine.java b/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachine.java index a9b9de4493..025edffc84 100644 --- a/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachine.java +++ b/resilience4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachine.java @@ -354,6 +354,9 @@ private void publishEventIfPossible(CircuitBreakerEvent event) { } private void publishStateTransitionEvent(final StateTransition stateTransition) { + if (StateTransition.isInternalTransition(stateTransition)) { + return; + } final CircuitBreakerOnStateTransitionEvent event = new CircuitBreakerOnStateTransitionEvent( name, stateTransition); publishEventIfPossible(event); diff --git a/resilience4j-circuitbreaker/src/test/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachineTest.java b/resilience4j-circuitbreaker/src/test/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachineTest.java index 1e972d801f..336bc80186 100644 --- a/resilience4j-circuitbreaker/src/test/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachineTest.java +++ b/resilience4j-circuitbreaker/src/test/java/io/github/resilience4j/circuitbreaker/internal/CircuitBreakerStateMachineTest.java @@ -625,15 +625,6 @@ public void shouldNotAllowTransitionFromClosedToHalfOpen() { assertThat(circuitBreaker.getState()).isEqualTo(CircuitBreaker.State.CLOSED); } - @Test - public void shouldNotAllowTransitionFromClosedToClosed() { - assertThatThrownBy(() -> circuitBreaker.transitionToClosedState()) - .isInstanceOf(IllegalStateTransitionException.class) - .hasMessage( - "CircuitBreaker 'testName' tried an illegal state transition from CLOSED to CLOSED"); - assertThat(circuitBreaker.getState()).isEqualTo(CircuitBreaker.State.CLOSED); - } - @Test public void shouldResetToClosedState() { circuitBreaker.transitionToOpenState(); @@ -723,6 +714,56 @@ public void shouldRecordMetricsInMetricsOnlyState() { assertThat(circuitBreaker.getState()).isEqualTo(CircuitBreaker.State.METRICS_ONLY); } + @Test + public void allCircuitBreakerStatesAllowTransitionToMetricsOnlyMode() { + for (final CircuitBreaker.State state : CircuitBreaker.State.values()) { + CircuitBreaker.StateTransition.transitionBetween(circuitBreaker.getName(), state, CircuitBreaker.State.METRICS_ONLY); + } + } + + @Test + public void allCircuitBreakerStatesAllowTransitionToItsOwnState() { + for (final CircuitBreaker.State state : CircuitBreaker.State.values()) { + CircuitBreaker.StateTransition.transitionBetween(circuitBreaker.getName(), state, state); + } + } + + @Test + public void circuitBreakerDoesNotPublishStateTransitionEventsForInternalTransitions() { + circuitBreaker.getEventPublisher().onStateTransition(mockOnStateTransitionEventConsumer); + int expectedNumberOfStateTransitions = 0; + + circuitBreaker.transitionToOpenState(); + expectedNumberOfStateTransitions++; + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + circuitBreaker.transitionToOpenState(); + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + + circuitBreaker.transitionToHalfOpenState(); + expectedNumberOfStateTransitions++; + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + circuitBreaker.transitionToHalfOpenState(); + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + + circuitBreaker.transitionToDisabledState(); + expectedNumberOfStateTransitions++; + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + circuitBreaker.transitionToDisabledState(); + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + + circuitBreaker.transitionToMetricsOnlyState(); + expectedNumberOfStateTransitions++; + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + circuitBreaker.transitionToMetricsOnlyState(); + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + + circuitBreaker.transitionToClosedState(); + expectedNumberOfStateTransitions++; + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + circuitBreaker.transitionToClosedState(); + verify(mockOnStateTransitionEventConsumer, times(expectedNumberOfStateTransitions)).consumeEvent(any(CircuitBreakerOnStateTransitionEvent.class)); + } + private void assertCircuitBreakerMetricsEqualTo(Float expectedFailureRate, Integer expectedSuccessCalls, Integer expectedBufferedCalls, Integer expectedFailedCalls, Long expectedNotPermittedCalls) {