Possibility to close connection

[jebl01]

Describe the feature

Vert.x 4

It should be possible to close the underlying connection during http calls

Use cases

When you e.g. use the reactive wrapper, or wrap it yourself, it would be nice to be able to close the connection if your reactive pipeline is being disposed.

Since the connection is hidden inside a HttpClientRequest inside the HttpContext this is easier said than done.

As I see it, two things needs to happen to make this possible;

1. expose the created HttpContext through the HttpRequest
2. don't null out the HttpClientRequest as soon as the connection has been made (the Java doc for HttpContext.clientRequest states: "Returns: the underlying client request, only available during ClientPhase.SEND_REQUEST and after", which is obviously not true

My experiment to try this out looks something like this:

public Single<HttpResponse<Buffer>> send(HttpRequest<Buffer> request) {
        return Single.create(emitter -> {

          //************* This replicates what happens in HttpRequestImpl.send(handler)
          final HttpContext<Buffer> context = webClient.createContext(HandlerUtils.fromSingleEmitter(emitter));
          context.prepareRequest(request, null, null);
          //***********************************************************************

          emitter.setCancellable(() -> {
            LoggerFactory.getLogger("HttpClient").info("CANCELLED, phase: " + context.phase());

            Optional.ofNullable(context.clientRequest()).ifPresent(r -> {
              //we never end up here, since the clientRequest is set to null in the context
              LoggerFactory.getLogger("HttpClient").info("CLOSING CONNECTION!!");
              r.connection().close();
            });
          });
        });
      }

Contribution

To me this sounds fairly easy to implement, but I'm not sure of the implications of not nulling out the clientRequest (and also what that would mean from pooling aspects etc). If the test coverage is good, I could help out with this...

[tsegismont]

The purpose of the web-client pool is to:

1. keep the number of connections under control
2. improve performance by maintaining long-lived connections

I'm not sure closing connections is compatible with these goals, in particular the second one.

If it's more important in your project's context to minimize resources used, why not handling connections manually?

[jebl01]

If you configure your WebClient with an idleTimeout, this is exactly what's going to happen (i.e. the connection will be closed). I would also expect the same behaviour if I used rxJava and configured a timeout that way.

The problem is that there is no way to cascade this timeout to other downstream services, e.g.: Service A calls Service B which calls Service C. When the call from Service A times out, the connection will be closed, but Service B and Service C will happily continue with whatever they are doing. Since it's possible to attach a closeHandler to the routingContext.response, it would be possible to fast and ruthlessly close outgoing calls from Service B (by closing the connection), effectively cascading the initial timeout throughout your entire system.

[tsegismont]

If you configure your WebClient with an idleTimeout, this is exactly what's going to happen (i.e. the connection will be closed). I would also expect the same behaviour if I used rxJava and configured a timeout that way.

I see. In fact, Vert.x will invoke reset on the HttpClientRequest, which effectively closes the connection for for HTTP/1.x but only sends a reset frame for HTTP/2. So, if we move forward with this proposal, we'd do the same.

The created HttpContext cannot be exposed through HttpRequest because these are meant to be reusable. We would need a new object that represents a specific exchange.

And then of course we need to understand why the HttpClientRequest is nulled-out early. This apparently comes from https://github.com/vert-x3/vertx-web/commit/d5566b0b43330b3f6940535f328125ec6d5d00f6#diff-85c06f23cdf1cba45b079b7d98f3d24646f7a6c47862cac5b5aa05b8daf43723

Do you remember the rationale behind this @vietj ?