Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibility to close connection #2579

Open
jebl01 opened this issue Mar 13, 2024 · 3 comments
Open

Possibility to close connection #2579

jebl01 opened this issue Mar 13, 2024 · 3 comments

Comments

@jebl01
Copy link

jebl01 commented Mar 13, 2024

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
Copy link
Contributor

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
Copy link
Author

jebl01 commented Mar 19, 2024

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
Copy link
Contributor

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 d5566b0#diff-85c06f23cdf1cba45b079b7d98f3d24646f7a6c47862cac5b5aa05b8daf43723

Do you remember the rationale behind this @vietj ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants