Race condition in BufferingClientHttpResponseWrapper.getBody() #35745
Description
Accessing BufferingClientHttpResponseWrapper.getBody() with an @Async method causes a race condition that leads to the second thread hanging. The issue is due to both threads getting inside the null check in BufferingClientHttpResponseWrapper.getBody():
if (this.body == null) {
this.body = StreamUtils.copyToByteArray(this.response.getBody());
}
The issue could be fixed by synchronizing the getBody() method or using a double null check with a nested synchronized block if you want to avoid synchronizing the entire method. I.e.:
if(this.body == null) {
synchronized (this.response) {
if(this.body == null) {
this.body = StreamUtils.copyToByteArray(this.response.getBody());
}
}
}
How to reproduce:
I am using a ClientHttpRequestInterceptor to log requests and responses to an external data store. The logging is done asynchronously to avoid waiting on the insert to the external store to complete before the request can be processed by the caller. Currently the race condition shows up in tests by hanging the test. Here is a sample of the ClientHttpRequestInterceptor that causes the issue:
@RequiredArgsConstructor
private static class LoggingInterceptor implements ClientHttpRequestInterceptor {
private final RequestLogService requestLogService;
@Override
public ClientHttpResponse intercept(HttpRequest request, byte[] body, ClientHttpRequestExecution execution) throws IOException {
//Get the Content-Type
String contentType = request.getHeaders().getContentType() != null ? request.getHeaders().getContentType().toString() : null;
//Log the request
CompletableFuture<OutgoingRequest> outgoingRequest = requestLogService.logOutgoingRequestAsync(
request.getURI(),
request.getMethod().name(),
contentType,
new String(body));
//Perform the request
ClientHttpResponse response = execution.execute(request, body);
//Log the response
requestLogService.logOutgoingResponseAsync(response, outgoingRequest);
return response;
}
}
How to work-around:
The current work-around is to read the request body in a single thread, which complicates the call to requestLogService.logOutgoingResponseAsync(response, outgoingRequest) somewhat and puts that preparation logic in the caller. This is also somewhat of a gotcha that took a while to track down. It would be nice if if was eliminated.