Ensure that WebClient disposes the HTTP client connection once the client response is consumed [SPR-15920]

[spring-projects-issues]

Brian Clozel opened SPR-15920 and commented

Problems with the WebClient API

Given the current WebClient API, it is possible to have:

Mono<String> result = this.webClient.get()
				.uri("/greeting")
				.retrieve()
				.bodyToMono(String.class);

In that case, we're consuming the response body completely; under the covers, reactor-netty will dispose the connection (close it or return it to the connection pool) as soon as the body has been consumed.

But we can also do this:

Mono<HttpStatus> status = this.webClient.get()
				.uri("/example")
				.exchange()
				.map(response -> response.statusCode());

In that case, the body is not consumed, and the underlying client has no way of knowing that the connection should be closed. This can lead to issues with the connection pool, memory leaks, etc.

In the ClientConnector#connect method, before returning the ClientHttpResponse, we could do something like:

responseMono.doOnTerminate((response, ex) -> {
  if (response != null) {
    response.dispose();
  }
})

But unfortunately, this will close the connection too soon. The first example can be rewritten like:

Mono<ResponseEntity<String>> result = this.webClient.get()
				.uri("/greeting")
				.exchange()
				.flatMap(response -> response.toEntity(String.class));

With the flatMap operator, we wait until the Mono<ClientHttpResponse> is completed and proceed with the body. The completion triggers that doOnTerminate operator.

Reactor Netty changes

Reactor Netty is currently dealing with this in its API and considering the following changes:

public interface ResponseReceiver  {
 
  // HttpClientResponse has no reference to the body, just headers/status/etc
  Mono<HttpClientResponse> response();

  <V> Flux<V> response(BiFunction<? super HttpClientResponse, ? super ByteBufFlux, ? extends Publisher<? extends V>> receiver);

  ByteBufFlux responseContent();

  <V> Mono<V> responseSingle(BiFunction<? super HttpClientResponse, ? super ByteBufMono, ? extends Mono<? extends V>> receiver);

}

With this type of changes, the response body Publisher is tied back to the lifecycle of the connection.

We need to revisit our current client API to make sure that the connection lifecycle can be properly managed for the underlying client library.

---

Issue Links:

• Allow Consumer-style access to FluxExchangeResult within chain of calls [SPR-15959] #20511 Allow Consumer-style access to FluxExchangeResult within chain of calls
• Revisit how WebClient disposes connection [SPR-15993] #20542 Revisit how WebClient disposes connection

[spring-projects-issues]

Brian Clozel commented

A few thoughts:

It looks like the Reactor Netty changes will be fine for end-users, but not ideal for a library usage, like it's the case right now in Spring Framework. Because of those changes, We'd need to know, at the ClientConnector level (the adapter for HTTP client libraries), if we are going to consume the body as byte buffers, or also decode it as a Mono or a Flux, or just read header.

I'm thinking about adding a new method to the client response: ClientHttpResponse.dispose().
Users would be responsible to call it as soon as they're not consuming the body (we could do it automatically as soon as you're consuming the body).

We could then rewrite the code sample like this

Mono<HttpStatus> status = this.webClient.get()
				.uri("/example")
				.exchange()
				.map(response -> {
					response.dispose();
					return response.statusCode();
				});

Or even avoid returning the ClientHttpResponse altogether and only the status+headers information (the HttpMessage interface). In that case, we could dispose automatically as well.

In fact, I'd even argue that being able to cancel manually and avoid receiving a response from the server might be an additional feature (that we considered in the past but couldn't properly implement with RestTemplate).

[spring-projects-issues]

Brian Clozel commented

rstoyanchev Sébastien Deleuze Arjen Poutsma

I think we should add that dispose() method to our API, knowing that its behavior will change depending on the HTTP version and client options. This might be even more important when the client will support HTTP/2 (dispose should close the current HTTP stream, since the connection is shared by all requests/responses).

/**
 * Release the HTTP-related resources.
 * <p>Depending on the client configuration and protocol,
 * this cas lead to closing the connection or returning it to the
 * connection pool.
 */
void dispose();

Adding it in the right place is more difficult. We should call it automatically as soon as we're consuming the response body through our API. DefaultWebClient.bodyToPublisher seems to be a right choice, but it's currently returning a raw Publisher when we need to use a doOnTerminate operator.

Taking a step back (and chatting with Sebastien about it), we can identity a few uses cases with the WebClient API:

1. client.retrieve().bodyTo*() to fetch the decoded response body
2. client.retrieve().bodyTo*().onStatus() to do the same with finer error handling
3. we may be missing an API to get only the response HttpMessage (headers and status) only, or both the decoded body and the metadata?
4. client.exchange() now gives you the responsibility to call dispose() if you're not consuming the response body. Maybe we should adapt the current API to push retrieve() as a preferred alternative?

[spring-projects-issues]

Brian Clozel commented

Updated status after a team call:

1. the current WebClient API needs to be improved because of resource management issues: we should have a way to signal that we're done with the response so that resources can be closed/recycled
2. the draft changes in reactor netty address that issue internally but can be problematic for Framework since it limits our ability to use it as a library
3. we're working on the relevant changes in Framework but we still have some design concerns

We should add a new method on ClientHttpResponse and we're hesitating between two:

• ClientHttpResponse#dispose() or ClientHttpResponse#close(); this method should be called if you're given the response object (no matter if you're reading the body or not). This will be called automatically if you're consuming the body through the retrieve() method. But does this mean we should be able to call that method at any time, even while reading the response body?
• ClientHttpResponse#ignoreBody(); this method is an alternative to the response.bodyTo* ones. It will close the resources properly under the covers, but this is not the main purpose of that API.

[hughwphamill]

@bclozel Was this ever resolved?

I'm trying to call out to an external authentication service using a HEAD request as part of a spring cloud gateway flow. I'm using exchange and want to just extract one of the headers if the response status is 200 and then pass the header along as part of the overall gateway call.

Ideally I'd like to log the authentication service body if there was an error, but I can live without it. Right now it seems like the connection is not properly disposed when read the header, because I don't do anything with the body.

[bclozel]

@hughwphamill yes, in the meantime we've added a documentation note on the javadoc for org.springframework.web.reactive.function.client.ClientResponse. For more details on that, there is also a more detailed answer: https://stackoverflow.com/questions/51312537/what-is-the-proper-way-to-make-an-api-call-via-springs-webclient-but-ignore-t/51321602#51321602

[hughwphamill]

@bclozel Thanks for the response, I have one follow up question.

My particular case has me sending a head request, on success there'll be no body, but on failure (e.g. a 500 error) there may be a body in the VndError format. Is it safe to consume the body as string in the success case, even when there'll be no body, or should i consume to void, and only consume the body as String in the event that there was a failure?

[rstoyanchev]

I believe for HEAD it's not necessary to read the body since a body isn't expected which the underlying client and sever know. That said there is not issue with consuming when there is no body, or you could consume to Void.class.

[hughwphamill]

@rstoyanchev Thanks, for my purposes I was getting a strange connection being held open issue, but I think it was because I was not actually consuming the mono as a producer, as it was in the middle of a different stream. I switched to RestTemplate in a blocking Mono and it fixed things up for me, thanks for the input.