From b2e7be17ab3180e055d4fd4b70dbe33c99b55c24 Mon Sep 17 00:00:00 2001
From: Brian Clozel <bclozel@pivotal.io>
Date: Thu, 12 Apr 2018 22:50:16 +0200
Subject: [PATCH] Polish gh-11514

As pointed out by Rossen in gh-11514 comments, a handler might commit
the response and then send an error signal in the pipeline. In this
case, adding a callback to `beforeCommit` is useless because it won't be
triggered. In those cases, we need to collect metrics right away.
---
 .../web/reactive/server/MetricsWebFilter.java    | 16 ++++++++++++----
 .../reactive/server/MetricsWebFilterTests.java   | 16 ++++++++++++++++
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java
index 8250b09a072..799bf0103d9 100644
--- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java
+++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilter.java
@@ -62,8 +62,17 @@ public class MetricsWebFilter implements WebFilter {
 		long start = System.nanoTime();
 		ServerHttpResponse response = exchange.getResponse();
 		return call.doOnSuccess((done) -> success(exchange, start))
-				.doOnError((cause) -> response
-						.beforeCommit(() -> error(exchange, start, cause)));
+				.doOnError((cause) -> {
+					if (response.isCommitted()) {
+						error(exchange, start, cause);
+					}
+					else {
+						response.beforeCommit(() -> {
+							error(exchange, start, cause);
+							return Mono.empty();
+						});
+					}
+				});
 	}
 
 	private void success(ServerWebExchange exchange, long start) {
@@ -72,11 +81,10 @@ public class MetricsWebFilter implements WebFilter {
 				TimeUnit.NANOSECONDS);
 	}
 
-	private Mono<Void>  error(ServerWebExchange exchange, long start, Throwable cause) {
+	private void error(ServerWebExchange exchange, long start, Throwable cause) {
 		Iterable<Tag> tags = this.tagsProvider.httpRequestTags(exchange, cause);
 		this.registry.timer(this.metricName, tags).record(System.nanoTime() - start,
 				TimeUnit.NANOSECONDS);
-		return Mono.empty();
 	}
 
 }
diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java
index 82cf4e6afd4..97540d8e7b9 100644
--- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java
+++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java
@@ -74,6 +74,22 @@ public class MetricsWebFilterTests {
 		assertMetricsContainsTag("status", "500");
 	}
 
+	@Test
+	public void filterAddsTagsToRegistryForExceptionsAndCommittedResponse() {
+		MockServerWebExchange exchange = createExchange("/projects/spring-boot",
+				"/projects/{project}");
+		this.webFilter.filter(exchange,
+				serverWebExchange -> {
+					exchange.getResponse().setStatusCodeValue(500);
+					return exchange.getResponse().setComplete()
+							.then(Mono.error(new IllegalStateException("test error")));
+				})
+				.onErrorResume(t -> Mono.empty())
+				.block();
+		assertMetricsContainsTag("uri", "/projects/{project}");
+		assertMetricsContainsTag("status", "500");
+	}
+
 	private MockServerWebExchange createExchange(String path, String pathPattern) {
 		PathPatternParser parser = new PathPatternParser();
 		MockServerWebExchange exchange = MockServerWebExchange