quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

Provides a way to configure HTTP Keep-Alive #6132

Open loicmathieu opened 4 years ago

loicmathieu commented 4 years ago

Description When I call my rest endpoint with the header Connection: keep-alive I have no response header Keep-Alive inside the response.

And it seems that the connection is not kept alive between multiple request as I saw a lot of TCP socket in TIMED_WAIT state.

I try to set TCP keep alive at Vert.x level but this seems to have no effect:

quarkus.vertx.eventbus.tcp-keep-alive=true

Implementation ideas Allow to configure keep alive (not only true/false but also the keep-alive timeout and max-age)

vietj commented 10 months ago

this configures TCP keep alive and not HTTP keep alive

yk-littlepay commented 1 month ago

@holly-cummins I do not know who to ask since it is a very old issue but is there any chances that this will be closed?, because as it turned out it an issue for AWS lambdas those are using Apache HttpClient . So the biggest issue that there is no expected headers Keep-Alive: timeout=60 and Connection : keep-alive in a response when a request contains the header Connection : keep-alive . PS. As far as I understand the quarkus.http.idle-timeout is responsible for the very keep-alive timeout

geoand commented 1 month ago

@vietj @cescoffier doesn't Vertx handle this automatically?

cescoffier commented 1 month ago

I don't think Vert.x does set the Keep-Alive header. Note that this header is forbidden with HTTP/2 and HTTP/3. If you want to set this header, you will have to do it manually (in a route or in your response), while making sure: 1) connection is set to keep-alive, 2) it's HTTP/1.0 or HTTP/1.1.

yk-littlepay commented 1 month ago

@cescoffier your right, but for HTTP/1.0 or HTTP/1.1. its still valid headers, and: 1) tell me honestly, how long do you think HTTP/1.0 or HTTP/1.1 will still be used? 2) as a library user, I expect it to comply with the previously agreed protocol, or at least I expect it to be specified in the documentation that there is some number of things I have to take care. 3) On the one hand, this is a small thing, but it is a very annoying little thing and causes periodic errors in logs that are quite difficult to reproduce and require exhausting analysis. 4) The same spring processed this header as expected, and when, after long discussions with the team, you finally manage to convince them to use something other than spring, then if such misunderstandings arise, it severely damages your reputation and all your subsequent arguments come down to zero with the simple phrase "It worked great with Spring. while here you have to manually manipulate the headers." As a result, the whole idea to transfer all services to a new framework is simply disappearing before eyes.

cescoffier commented 1 month ago

My note was just to ensure you do not write that header when using HTTP/2 or HTTP/3 (as the client may consider the response invalid).

As said, there is no way to "generate" that header in Quarkus, but you can still pass it manually. You can add an interceptor or update the response/request to set it if necessary.

I think there is a way to set it on the HTTP client but not on the server. I would add some words of caution: Some infrastructure technologies recycle connections regardless of the application setup. Thus, that information would be misleading, as the infrastructure would cut the connection anyway (regardless of the "agreed protocol," as you said) after some (generally too low) idle timeout (~10s seems to be the norm).

Can you explain if you need these headers on the HTTP request or response (the header can be used on both)? Then, I can try to explain how they can be set.

yk-littlepay commented 1 month ago

I think there is a way to set it on the HTTP client but not on the server.

That's correct, it is surprising why this is possible for the client, but impossible for the server, it is even more misleading

Can you explain if you need these headers on the HTTP request or response (the header can be used on both)

My case is quite similar to this SSLException, SocketException : Connection reset from Apache HttpClient, except that we have some aws lambda (that uses apache client) that is periodically triggered by an event and after a while it just crashed with a SocketException. PS. In a good way, it would be necessary to update all services using new approaches and libraries, but it is impossible to do everything at once in one fell swoop. (Let's be honest, some services will remain not updated forever)

cescoffier commented 1 month ago

So, you have a http API and you want to set these headers. Which technology do you use? Quarkus REST, reactive routes?

yk-littlepay commented 1 month ago

Let's say I have a quarkus-rest-jackson + mutiny application that is replacement for springboot application . As mentioned above, to ensure a smooth transition to quarkus I had to add some class like this

package com.somecompany.security.http;

import jakarta.annotation.Nonnull;
import jakarta.ws.rs.container.ContainerRequestContext;
import jakarta.ws.rs.container.ContainerRequestFilter;
import jakarta.ws.rs.container.ContainerResponseContext;
import jakarta.ws.rs.container.ContainerResponseFilter;
import jakarta.ws.rs.ext.Provider;
import lombok.val;

/**
 * A filter that adds the necessary headers to the response to keep the connection alive.
 *
 * @implNote This is a workaround for the issue with the HTTP client not closing the connection after the response is received.
 * some related links:
 * <a href="https://github.com/quarkusio/quarkus/issues/6132">No corresponding headers in a response</a>
 * <a href="https://stackoverflow.com/questions/71126816/sslexception-socketexception-connection-reset-from-apache-httpclient">SocketException : Connection reset from Apache HttpClient</a>
 * @implSpec it's hardcoded to keep the connection alive for 60 seconds. because the {@code quarkus.http.idle-timeout} is 1m by
 * default, and it's not configurable at the moment.
 */
@Provider
public final class HeaderResponseFilter implements ContainerResponseFilter {
    private static final String HEADER_KEY_CONNECTION = "Connection";
    private static final String HEADER_KEY_KEEP_ALIVE = "Keep-Alive";
    private static final String HEADER_VALUE_KEEP_ALIVE = "keep-alive";
    private static final String HEADER_VALUE_TIMEOUT = "timeout=60";

    @Override
    public void filter(@Nonnull ContainerRequestContext requestContext, @Nonnull ContainerResponseContext responseContext) {
        // If keep-alive was requested, add the necessary headers to the response
        val connectionHeaderValue = requestContext.getHeaderString(HEADER_KEY_CONNECTION);
        if (HEADER_VALUE_KEEP_ALIVE.equalsIgnoreCase(connectionHeaderValue)) {
            val responseHeaders = responseContext.getHeaders();
            if (!responseHeaders.containsKey(HEADER_KEY_KEEP_ALIVE)) {
                responseHeaders.add(HEADER_KEY_CONNECTION, HEADER_VALUE_KEEP_ALIVE);
                responseHeaders.add(HEADER_KEY_KEEP_ALIVE, HEADER_VALUE_TIMEOUT);
            }
        }
    }
}
cescoffier commented 1 month ago

Yes, that looks correct. However, you should check the HTTP version too (for the reason I mentioned above)

You set the timeout to 60. Why this value? It should be the configured idle timeout.

yk-littlepay commented 1 month ago

You set the timeout to 60. Why this value? It should be the configured idle timeout.

Could you please provide the version that, in your opinion, is the most correct, and why

cescoffier commented 1 month ago

The default idle timeout is 30min:

    /**
     * Http connection idle timeout
     */
    @ConfigItem(defaultValue = "30M", name = "idle-timeout")
    public Duration idleTimeout;

So, I would have injected that property:

@ConfigProperty(name="quarkus.http.idle-timeout", defaultValue="30M") Duration idleTimeout;

Then:

 @Override
    public void filter(@Nonnull ContainerRequestContext requestContext) {
        // Check if the incoming request has the Connection: keep-alive header
        val connectionHeader = requestContext.getHeaderString(HEADER_CONNECTION);
        // TODO Check if HTTP/1 or 1.1
        keepAliveRequested = HEADER_VALUE_KEEP_ALIVE.equalsIgnoreCase(connectionHeader);
    }

@Override
    public void filter(@Nonnull ContainerRequestContext requestContext, @Nonnull ContainerResponseContext responseContext) {
        // If keep-alive was requested, add the necessary headers to the response
        if (keepAliveRequested) {           
            val responseHeaders = responseContext.getHeaders();
            responseHeaders.add(HEADER_CONNECTION, HEADER_VALUE_KEEP_ALIVE);
            responseHeaders.add(HEADER_KEEP_ALIVE,"timeout=" +  idleTimeout.toSeconds());
        }
    }

(pseudo code)

You are using a filter. If performance is a concern, I believe a route should be slightly faster (a bit lower level).