Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-15920

Ensure that WebClient disposes the HTTP client connection once the client response is consumed

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Complete
    • Affects Version/s: None
    • Fix Version/s: 5.0 RC4
    • Component/s: Reactive, Web:Client
    • Labels:
      None
    • Last commented by a User:
      false

      Description

      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

          Activity

          Hide
          bclozel Brian Clozel added a comment -

          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).

          Show
          bclozel Brian Clozel added a comment - 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 ).
          Hide
          bclozel Brian Clozel added a comment -

          Rossen Stoyanchev 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?
          Show
          bclozel Brian Clozel added a comment - Rossen Stoyanchev 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: client.retrieve().bodyTo*() to fetch the decoded response body client.retrieve().bodyTo*().onStatus() to do the same with finer error handling we may be missing an API to get only the response HttpMessage (headers and status) only, or both the decoded body and the metadata? 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?
          Hide
          bclozel Brian Clozel added a comment -

          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.
          Show
          bclozel Brian Clozel added a comment - Updated status after a team call: 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 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 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.

            People

            • Assignee:
              bclozel Brian Clozel
              Reporter:
              bclozel Brian Clozel
              Last updater:
              Rossen Stoyanchev
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                10 weeks, 3 days ago