search

eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.31k stars 2.08k forks source link

HttpClient handles HTTP/2 connection closing more gracefully #4264

Closed flore77 closed 2 years ago

flore77 commented 2 years ago

Describe the feature

We are using Vertx HttpClient and we noticed that when a server closes the connection via a GOAWAY frame, the in flight streams with id lower than the Last-Stream-Id returned in the GOAWAY frame are closed and a CONNECTION_CLOSED exception is thrown for each one of them.

As from our understanding this flow is triggered also from Netty's channelInactive method, so from a high level it is not clear if there was an actual error, such as an ECONNRESET, our it is just normal operation of HTTP/2 connection closing.

So, we are thinking of 2 options:

  1. Maybe the HttpClient should retry these requests itself? i.e. sending them on another connection, as it is normal operation, no exceptional behaviour.
  2. Or at least to throw another exception which will clearly state the reason why the given stream was not correctly processed. This is important because it will let the developer know that it can safely retry it, as the stream was not processed by the other part. Where in case of an ECONNRESET you cannot know if the other part received the stream and processed it successfully.

What do you think?

Use cases

Depending on the chosen solution:

Contribution

Happy to contribute

vietj commented 2 years ago

with should be option 2, as the client is not doing retries, instead it let the client perform it

flore77 commented 2 years ago

Thanks for the quick reply! Indeed the client is not doing retries, but I was thinking that maybe in this case it should, as one could argue that this is not actually a retry as no exception happened, it is just normal HTTP/2 operation. Nevertheless, I am fine also with option 2 to just throw a different exception.

vietj commented 2 years ago

have you tried using the goAwayHandler to detect when this happen ?

flore77 commented 2 years ago

We didn't use the goAwayHandler, because in the first place we didn't know that they are closing due to a GOAWAY frame, also not sure how we can register the handle via the HttpClient as it is handling the connections behind the scenes?

We investigated and this is happening when we are talking to a service which uses Nginx, which has a maximum number of requests that can be served through a connection (see http://nginx.org/en/docs/http/ngx_http_core_module.html#keepalive_requests), after which it closes the connection. Which is fine, as it is sending us a GOAWAY frame as the protocol dictates, only that we ended up seeing constant io.vertx.core.VertxException: Connection was closed and this triggered us to investigate as it was not clear what was triggering the closure.

So, this is why we are thinking to at least throw a different error, or maybe even send those streams (requests) on another connection automatically without requiring the developer to take extra measures.

vietj commented 2 years ago

I think we should avoid logging connection closed like we do (somebody reported that recently) as this is a normal event in such situation.

is nginx doing a graceful shutdown with a grace period for HTTP streams or are streams also closed ?

I think in such situation we should evict (on a go away), the HTTP/2 connection from the pool or make it as unusable.

flore77 commented 2 years ago

I think we should avoid logging connection closed like we do (somebody reported that recently) as this is a normal event in such situation.

Totally agree.

is nginx doing a graceful shutdown with a grace period for HTTP streams or are streams also closed ?

Nginx is doing a graceful shutdown in the sense that it is sending us a GOAWAY frame with the last processed stream id, but it is not doing this with a grace period, meaning that it is not telling us in advance, it just closes the connection when it processes the last stream (per its configuration), so other in flight streams are also closed, and the connection closed exception is thrown for every stream.

I think in such situation we should evict (on a go away), the HTTP/2 connection from the pool or make it as unusable.

I think the HTTP/2 connection is evicted from the pool, we didn't notice a behaviour in which we borrow a connection and it was closed. As a side note, this could happen for HTTP/1 connection pool, if the keepAlive setting is bigger than the upstream's, because HTTP/1 has no mechanism in place to notify the other part, but I don't think it is something that we can do in Vertx about it, the user just needs to match the keepAlive configurations (client keepAlive < upstream keepAlive).

vietj commented 2 years ago

looking back at this, can you describe the precise behavior of nginx in this case ? as far as I understand it sends a go away frame and then close the TCP connection, is that right ?

vietj commented 2 years ago

I just wrote a simple test to reproduce and indeed the client HttpConnection receives the go away event and then the streams are closed when the TCP connection closes. It seems to be the expected behavior as the client can set a go away handler and react to it as the TCP connection close might be delayed by the server (which is not in this case).

I noticed that when it happens, the HTTP connection is actually evicted from the pool.

We can improve the HttpConnection API to record the go away status and provide it, e.g something like HttpConnection#isGoneAway that the developer can use to determine that the HttpConnection received a go away frame.

vietj commented 2 years ago

https://github.com/eclipse-vertx/vert.x/pull/4313