Skip to content

Commit 72c8e5d

Browse files
bclozelbclozel
committed
Collect HTTP trace at commit time for WebFlux
Prior to this commit, the `HttpTraceWebFilter` would collect the response information (status and headers) for tracing purposes, after the handling chain is done with the exchange - inside a `doAfterSuccessOrError`. Once the handler has processed the exchange, there is no strong guarantee about the HTTP resources being still present. Depending on the web server implementation, HTTP resources (including HTTP header maps) might be recycled, because pooled in the first place. This commit moves the collection and processing of the HTTP trace right before the response is committed. This removes the need to handle special cases with exceptions, since by that time all exception handlers have processed the response and the information that we extract is the information that's about to be written to the network. Fixes spring-projectsgh-15819
1 parent cba6079 commit 72c8e5d

File tree

3 files changed

+30
-60
lines changed

3 files changed

+30
-60
lines changed

‎spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/trace/reactive/HttpTraceWebFilter.java

+6-28
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2012-2018 the original author or authors.
2+
* Copyright 2012-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -26,10 +26,6 @@
2626
import org.springframework.boot.actuate.trace.http.HttpTraceRepository;
2727
import org.springframework.boot.actuate.trace.http.Include;
2828
import org.springframework.core.Ordered;
29-
import org.springframework.http.HttpStatus;
30-
import org.springframework.http.server.reactive.ServerHttpResponse;
31-
import org.springframework.http.server.reactive.ServerHttpResponseDecorator;
32-
import org.springframework.web.server.ResponseStatusException;
3329
import org.springframework.web.server.ServerWebExchange;
3430
import org.springframework.web.server.WebFilter;
3531
import org.springframework.web.server.WebFilterChain;
@@ -95,38 +91,20 @@ private Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain,
9591
Principal principal, WebSession session) {
9692
ServerWebExchangeTraceableRequest request = new ServerWebExchangeTraceableRequest(
9793
exchange);
98-
HttpTrace trace = this.tracer.receivedRequest(request);
99-
return chain.filter(exchange).doAfterSuccessOrError((aVoid, ex) -> {
94+
final HttpTrace trace = this.tracer.receivedRequest(request);
95+
exchange.getResponse().beforeCommit(() -> {
10096
TraceableServerHttpResponse response = new TraceableServerHttpResponse(
101-
(ex != null) ? new CustomStatusResponseDecorator(ex,
102-
exchange.getResponse()) : exchange.getResponse());
97+
exchange.getResponse());
10398
this.tracer.sendingResponse(trace, response, () -> principal,
10499
() -> getStartedSessionId(session));
105100
this.repository.add(trace);
101+
return Mono.empty();
106102
});
103+
return chain.filter(exchange);
107104
}
108105

109106
private String getStartedSessionId(WebSession session) {
110107
return (session != null && session.isStarted()) ? session.getId() : null;
111108
}
112109

113-
private static final class CustomStatusResponseDecorator
114-
extends ServerHttpResponseDecorator {
115-
116-
private final HttpStatus status;
117-
118-
private CustomStatusResponseDecorator(Throwable ex, ServerHttpResponse delegate) {
119-
super(delegate);
120-
this.status = (ex instanceof ResponseStatusException)
121-
? ((ResponseStatusException) ex).getStatus()
122-
: HttpStatus.INTERNAL_SERVER_ERROR;
123-
}
124-
125-
@Override
126-
public HttpStatus getStatusCode() {
127-
return this.status;
128-
}
129-
130-
}
131-
132110
}

‎spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/web/trace/reactive/TraceableServerHttpResponse.java

+11-7
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2012-2018 the original author or authors.
2+
* Copyright 2012-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -30,21 +30,25 @@
3030
*/
3131
class TraceableServerHttpResponse implements TraceableResponse {
3232

33-
private final ServerHttpResponse response;
33+
private final int status;
34+
35+
private final Map<String, List<String>> headers;
36+
37+
TraceableServerHttpResponse(ServerHttpResponse response) {
38+
this.status = (response.getStatusCode() != null)
39+
? response.getStatusCode().value() : 200;
40+
this.headers = new LinkedHashMap<>(response.getHeaders());
3441

35-
TraceableServerHttpResponse(ServerHttpResponse exchange) {
36-
this.response = exchange;
3742
}
3843

3944
@Override
4045
public int getStatus() {
41-
return (this.response.getStatusCode() != null)
42-
? this.response.getStatusCode().value() : 200;
46+
return this.status;
4347
}
4448

4549
@Override
4650
public Map<String, List<String>> getHeaders() {
47-
return new LinkedHashMap<>(this.response.getHeaders());
51+
return this.headers;
4852
}
4953

5054
}

‎spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/trace/http/reactive/HttpTraceWebFilterTests.java

+13-25
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,10 @@
1616

1717
package org.springframework.boot.actuate.trace.http.reactive;
1818

19-
import java.io.IOException;
2019
import java.security.Principal;
2120
import java.time.Duration;
2221
import java.util.EnumSet;
2322

24-
import javax.servlet.ServletException;
25-
2623
import org.junit.Test;
2724
import reactor.core.publisher.Mono;
2825

@@ -33,10 +30,11 @@
3330
import org.springframework.boot.actuate.web.trace.reactive.HttpTraceWebFilter;
3431
import org.springframework.mock.http.server.reactive.MockServerHttpRequest;
3532
import org.springframework.mock.web.server.MockServerWebExchange;
33+
import org.springframework.web.server.ServerWebExchange;
3634
import org.springframework.web.server.ServerWebExchangeDecorator;
35+
import org.springframework.web.server.WebFilterChain;
3736

3837
import static org.assertj.core.api.Assertions.assertThat;
39-
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
4038
import static org.mockito.BDDMockito.given;
4139
import static org.mockito.Mockito.mock;
4240

@@ -56,18 +54,17 @@ public class HttpTraceWebFilterTests {
5654
this.tracer, EnumSet.allOf(Include.class));
5755

5856
@Test
59-
public void filterTracesExchange() throws ServletException, IOException {
60-
this.filter.filter(
57+
public void filterTracesExchange() {
58+
executeFilter(
6159
MockServerWebExchange
6260
.from(MockServerHttpRequest.get("https://api.example.com")),
6361
(exchange) -> Mono.empty()).block(Duration.ofSeconds(30));
6462
assertThat(this.repository.findAll()).hasSize(1);
6563
}
6664

6765
@Test
68-
public void filterCapturesSessionIdWhenSessionIsUsed()
69-
throws ServletException, IOException {
70-
this.filter.filter(
66+
public void filterCapturesSessionIdWhenSessionIsUsed() {
67+
executeFilter(
7168
MockServerWebExchange
7269
.from(MockServerHttpRequest.get("https://api.example.com")),
7370
(exchange) -> {
@@ -82,9 +79,8 @@ public void filterCapturesSessionIdWhenSessionIsUsed()
8279
}
8380

8481
@Test
85-
public void filterDoesNotCaptureIdOfUnusedSession()
86-
throws ServletException, IOException {
87-
this.filter.filter(
82+
public void filterDoesNotCaptureIdOfUnusedSession() {
83+
executeFilter(
8884
MockServerWebExchange
8985
.from(MockServerHttpRequest.get("https://api.example.com")),
9086
(exchange) -> {
@@ -97,10 +93,10 @@ public void filterDoesNotCaptureIdOfUnusedSession()
9793
}
9894

9995
@Test
100-
public void filterCapturesPrincipal() throws ServletException, IOException {
96+
public void filterCapturesPrincipal() {
10197
Principal principal = mock(Principal.class);
10298
given(principal.getName()).willReturn("alice");
103-
this.filter.filter(new ServerWebExchangeDecorator(MockServerWebExchange
99+
executeFilter(new ServerWebExchangeDecorator(MockServerWebExchange
104100
.from(MockServerHttpRequest.get("https://api.example.com"))) {
105101

106102
@Override
@@ -120,17 +116,9 @@ public Mono<Principal> getPrincipal() {
120116
assertThat(tracedPrincipal.getName()).isEqualTo("alice");
121117
}
122118

123-
@Test
124-
public void statusIsAssumedToBe500WhenChainFails()
125-
throws ServletException, IOException {
126-
assertThatExceptionOfType(Exception.class).isThrownBy(() -> this.filter
127-
.filter(MockServerWebExchange
128-
.from(MockServerHttpRequest.get("https://api.example.com")),
129-
(exchange) -> Mono.error(new RuntimeException()))
130-
.block(Duration.ofSeconds(30)));
131-
assertThat(this.repository.findAll()).hasSize(1);
132-
assertThat(this.repository.findAll().get(0).getResponse().getStatus())
133-
.isEqualTo(500);
119+
private Mono<Void> executeFilter(ServerWebExchange exchange, WebFilterChain chain) {
120+
return this.filter.filter(exchange, chain)
121+
.then(Mono.defer(() -> exchange.getResponse().setComplete()));
134122
}
135123

136124
}

0 commit comments

Comments
 (0)