Issue ReactiveX#480: Fixed a bug that CompletiongStage<Void> wasn't h…

…andled co… (ReactiveX#484) * Issue ReactiveX#480: Fixed a bug that CompletiongStage<Void> wasn't handled correctly in CircuitBreaker and Retry as the result was always null.
jihoonson · Jun 7, 2019 · b4f8b65 · b4f8b65
1 parent 30bb4a7
commit b4f8b65
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 14 deletions.
diff --git a/...4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/CircuitBreaker.java b/...4j-circuitbreaker/src/main/java/io/github/resilience4j/circuitbreaker/CircuitBreaker.java
@@ -475,15 +475,14 @@ static <T> Supplier<CompletionStage<T>> decorateCompletionStage(
                 try {
                     supplier.get().whenComplete((result, throwable) -> {
                         long durationInNanos = System.nanoTime() - start;
-                        if (result != null) {
+                        if(throwable != null){
+                            if(throwable instanceof Exception){
+                                circuitBreaker.onError(durationInNanos, throwable);
+                            }
+                            promise.completeExceptionally(throwable);
+                        }else{
                             circuitBreaker.onSuccess(durationInNanos);
                             promise.complete(result);
-                        } else if (throwable instanceof Exception) {
-                            circuitBreaker.onError(durationInNanos, throwable);
-                            promise.completeExceptionally(throwable);
-                        } else {
-                            // Do not handle java.lang.Error
-                            promise.completeExceptionally(throwable);
                         }
                     });
                 }catch (Exception exception){

diff --git a/...ircuitbreaker/src/test/java/io/github/resilience4j/circuitbreaker/CircuitBreakerTest.java b/...ircuitbreaker/src/test/java/io/github/resilience4j/circuitbreaker/CircuitBreakerTest.java
@@ -747,6 +747,27 @@ public void shouldExecuteCompletionStageAndReturnWithSuccess() throws ExecutionE
         assertThat(metrics.getNumberOfFailedCalls()).isEqualTo(0);
     }
 
+    @Test
+    public void shouldExecuteVoidCompletionStageAndReturnWithSuccess() throws ExecutionException, InterruptedException {
+        // Given
+        CircuitBreaker circuitBreaker = CircuitBreaker.ofDefaults("backendName");
+        assertThat(circuitBreaker.getState()).isEqualTo(CircuitBreaker.State.CLOSED);
+
+        // When
+
+        CompletionStage<Void> decoratedCompletionStage = circuitBreaker
+                .executeCompletionStage(() -> CompletableFuture.runAsync(helloWorldService::sayHelloWorld));
+
+        decoratedCompletionStage.toCompletableFuture().get();
+
+        // Then the helloWorldService should be invoked 1 time
+        BDDMockito.then(helloWorldService).should(Mockito.times(1)).sayHelloWorld();
+
+        CircuitBreaker.Metrics metrics = circuitBreaker.getMetrics();
+        assertThat(metrics.getNumberOfSuccessfulCalls()).isEqualTo(1);
+        assertThat(metrics.getNumberOfFailedCalls()).isEqualTo(0);
+    }
+
     @Test
     public void shouldDecorateCompletionStageAndReturnWithExceptionAtSyncStage() {
         // Given
@@ -864,4 +885,26 @@ public void testCreateWithNullConfig() {
         assertThatThrownBy(() -> CircuitBreaker.of("test", (CircuitBreakerConfig)null)).isInstanceOf(NullPointerException.class).hasMessage("Config must not be null");
     }
 
+    @Test
+    public void shouldNotMeasureErrorsAsFailures() {
+        // Given
+        CircuitBreaker circuitBreaker = CircuitBreaker.ofDefaults("testName");
+        CircuitBreaker.Metrics metrics = circuitBreaker.getMetrics();
+        // Given the HelloWorldService throws an exception
+        BDDMockito.given(helloWorldService.returnHelloWorld()).willThrow(new StackOverflowError("BAM!"));
+
+        //When
+        Supplier<String> supplier = CircuitBreaker.decorateSupplier(circuitBreaker, helloWorldService::returnHelloWorld);
+
+        assertThatThrownBy(supplier::get).isInstanceOf(StackOverflowError.class);
+
+        //Then
+        assertThat(metrics.getNumberOfBufferedCalls()).isEqualTo(0);
+        assertThat(metrics.getNumberOfFailedCalls()).isEqualTo(0);
+        assertThat(metrics.getNumberOfSuccessfulCalls()).isEqualTo(0);
+        // Then the helloWorldService should be invoked 1 time
+        BDDMockito.then(helloWorldService).should(Mockito.times(1)).returnHelloWorld();
+
+    }
+
 }
diff --git a/resilience4j-retry/src/main/java/io/github/resilience4j/retry/Retry.java b/resilience4j-retry/src/main/java/io/github/resilience4j/retry/Retry.java
@@ -479,14 +479,15 @@ class AsyncRetryBlock<T> implements Runnable {
 		public void run() {
 			final CompletionStage<T> stage = supplier.get();
 
-			stage.whenComplete((result, t) -> {
-				if (result != null) {
+			stage.whenComplete((result, throwable) -> {
+				if(throwable != null){
+					if(throwable instanceof Exception){
+						onError((Exception) throwable);
+					}else{
+						promise.completeExceptionally(throwable);
+					}
+				}else{
 					onResult(result);
-				} else if (t instanceof Exception) {
-					onError((Exception) t);
-				} else{
-					// Do not handle java.lang.Error
-					promise.completeExceptionally(t);
 				}
 			});
 		}

diff --git a/...j-retry/src/test/java/io/github/resilience4j/retry/internal/CompletionStageRetryTest.java b/...j-retry/src/test/java/io/github/resilience4j/retry/internal/CompletionStageRetryTest.java
@@ -69,6 +69,25 @@ public void shouldNotRetry() {
 		Assertions.assertThat(result).isEqualTo("Hello world");
 	}
 
+	@Test
+	public void shouldNotRetryWhenReturnVoid() {
+		BDDMockito.given(helloWorldService.sayHelloWorld())
+				.willReturn(completedFuture(null));
+
+		// Create a Retry with default configuration
+		Retry retryContext = Retry.ofDefaults("id");
+		// Decorate the invocation of the HelloWorldService
+		Supplier<CompletionStage<Void>> supplier = Retry.decorateCompletionStage(
+				retryContext,
+				scheduler,
+				() -> helloWorldService.sayHelloWorld());
+
+		// When
+		awaitResult(supplier);
+		// Then the helloWorldService should be invoked 1 time
+		BDDMockito.then(helloWorldService).should(Mockito.times(1)).sayHelloWorld();
+	}
+
 	@Test
 	public void shouldNotRetryWithThatResult(){
 		// Given the HelloWorldService returns Hello world