Armeria: Send `OPTIONS *` request when connection is idle in HTTP/1

[trustin]

When an intermediary load balancer proxies a request to an endpoint, an Armeria client that sends a request via the load balancer will sometimes get a ClosedSessionException due to a race condition like the following:

• The LB thinks the connection A is idle and decides to close the connection.
• Armeria client thinks it can reuse the connection A and sends the request to the connection A.
• The LB closes the connection.
• Armeria client gets a ClosedSessionException.

To fix the problem, a user can increase the idle timeout of the load balancer, but we could also send something similar to HTTP/2 PING frames, such as OPTIONS * HTTP/1.1, on an idle HTTP/1 connection with no active streams.

We will be able to reuse @sivaalli's work.

I think we are going to have to make the following two changes:

• Remove the sendPingsOnNoActiveStreams flag and send pings even if there are no active streams, so that an idle HTTP/2 connections are protected from the same problem.
  
  
  
  • Instead, allow a user to specify the interval between pings, which is expected to be less than the idle timeout.
  
  
  • As a result, we are going to have to keep track of two idle states, by using two IdleStateHandler or by forking IdleStateHandler for efficiency.
  
  
• When there are no requests in progress in an HTTP/1 connection, send a OPTIONS * HTTP/1.1 request as a ping to keep the connection alive.
  
  
  
  • The connection should still be closed when a user does not send requests until idle timeout occurs.
  
  

The first change is not necessary part of this issue, but I guess it will be much easier to implement the second change with the first one.

[anuraaga]

Remove the sendPingsOnNoActiveStreams flag and send pings even if there are no active streams, so that an idle HTTP/2 connections are protected from the same problem.

I don't think HTTP/2 should have the same problem because of go away. If we remove the flag, how can a user configure so idle HTTP/2 connections are closed when there aren't any streams?

When there are no requests in progress in an HTTP/1 connection, send a OPTIONS * HTTP/1.1 request as a ping to keep the connection alive.

Since an HTTP/1 connection can only handle one request at a time, perhaps it's simpler to special case the implementation for it? IIUC, we could just send options while ab HTTP/1 connection is sitting in the connection pool.

[trustin]

I don't think HTTP/2 should have the same problem because of go away.

But It will reduce the chance of getting UnprocessedRequestException(GoAwayReceivedException).

If we remove the flag, how can a user configure so idle HTTP/2 connections are closed when there aren't any streams?

We keep two idle state - one for pinging and the other for idle connections (the latter ignores the ping frames)

Since an HTTP/1 connection can only handle one request at a time, perhaps it's simpler to special case the implementation for it?

That's also possible, but doesn't it scatter the pinging logic?

[anuraaga]

But It will reduce the chance of getting UnprocessedRequestException(GoAwayReceivedException).

Hmm - this doesn't sound right to me. If the connection has already gone away, future requests should try a new connection instead of being failed right away by this exception. I think that's the point of goaway. Is this something we need to fix in our client?

We keep two idle state - one for pinging and the other for idle connections (the latter ignores the ping frames)

Ah I think right now we only ping if idle timeout is defined but this means flag-wise ping and idle timeout are completely decoupled? That seems nice.

That's also possible, but doesn't it scatter the pinging logic?

Not sure how much it scatters - HTTP/2 and 1 are so different that this handling of idleness seems like it should be special cased for each so it's optimal. For example, I still think the existence of goaway is a huge game changer between the two. If this hypothesis that they are very different is true is correct anyways, I'm feeling that the connection pool would be a good layer for HTTP/1. It does make me wonder perhaps it's the pool itself that should be special cased for each protocol.

[trustin]

Hmm - this doesn't sound right to me. If the connection has already gone away, future requests should try a new connection instead of being failed right away by this exception. I think that's the point of goaway. Is this something we need to fix in our client?

Yeah, we can retry, but isn't it better keeping the connection alive rather than opening a new one after seeing a disconnection? It'll be nice to have such an option for users.

Not sure how much it scatters - HTTP/2 and 1 are so different that this handling of idleness seems like it should be special cased for each so it's optimal.

Maybe. I guess I'll figure out while implementing.

[anuraaga]

Yeah, we can retry, but isn't it better keeping the connection alive rather than opening a new one after seeing a disconnection? It'll be nice to have such an option for users.

Yeah option to keep the connection alive sounds good - I was concerned more that we might not be handling goaway properly though if it's possible for a LB closing the connection causing a user to see an exception, I thought goaway is designed to prevent this from happening.

[trustin]

I was concerned more that we might not be handling goaway properly though if it's possible for a LB closing the connection causing a user to see an exception.

That would be a different issue. I'm actually investigating it as well internally. :smile: