Added support for grpc max_connection_age_ms

[ionut-slaveanu]

Motivation

See:

• Add support for MAX_CONNECTION_AGE and MAX_CONNECTION_AGE_GRACE equivalents #1428

Related PRs:

• Change graceful_shutdown function behavior. hyper#3729

Solution

Added a new configuration parameter: max_connection_age_ms.
Example:

Server::builder()
.max_connection_age_ms(5)
.add_service(GreeterServer::new(greeter))
.serve(addr)
.await?;

I'm using max_connection_age_ms set to 5 ms only for test purposes. In production it should be set to a greater value.

See:

• https://github.com/ionut-slaveanu/tonic/tree/master/examples/src/max_connection_age

We try to gracefully close each connection to the server after max_connection_age_ms milliseconds have passed.

Testing

Run:
./target/release/max-connection-age-server as grpc server.
As a client, i'm using ghz:

• https://ghz.sh/

Example of generating requests with:
./ghz --insecure --proto /opt/work3/tonic/examples/proto/helloworld/helloworld.proto --call helloworld.Greeter.SayHello -d '{"name":"Tonic"}' [::1]:50051 -n 100000 --connections=500 --cpus=16 --concurrency=500

Summary:
Count: 100184
Total: 2.62 s
Slowest: 1.11 s
Fastest: 0.04 ms
Average: 8.30 ms
Requests/sec: 38309.90

Response time histogram:
0.038 [1] |
111.425 [99908] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
222.813 [26] |
334.201 [0] |
445.588 [0] |
556.976 [0] |
668.363 [0] |
779.751 [0] |
891.139 [0] |
1002.526 [0] |
1113.914 [65] |

Latency distribution:
10 % in 0.38 ms
25 % in 0.88 ms
50 % in 2.09 ms
75 % in 8.35 ms
90 % in 25.62 ms
95 % in 34.60 ms
99 % in 55.86 ms

Status code distribution:
[OK] 100000 responses
[Unavailable] 184 responses

Error distribution:
[91] rpc error: code = Unavailable desc = the connection is draining
[93] rpc error: code = Unavailable desc = transport is closing

[djc]

Marking this as draft until upstream changes have been merged.

[ionut-slaveanu]

Fixed workflow:

Also removed the dependency on:
[patch.crates-io]
hyper = {git = "https://github.com/ionut-slaveanu/hyper.git" }

So, this pull request can be merged independently of the PR on hyper.

[djc]

So is this even needed if hyperium/hyper#3729 is merged?

[tottoto]

Can this config take Duration instead of u64 ms?

[djc]

Can this config take Duration instead of u64 ms?

Good idea -- if we do that (pending whether it's needed after hyper implements this), should also get rid of the _ms suffix.

[ionut-slaveanu]

Can this config take Duration instead of u64 ms?

Good idea -- if we do that (pending whether it's needed after hyper implements this), should also get rid of the _ms suffix.

Fixed,

[ionut-slaveanu]

So is this even needed if hyperium/hyper#3729 is merged?

The changes present in hyperium/hyper#3729 are on my fork.
So, the following code is the same as the one from hyper PR:
[patch.crates-io]
hyper = {git = "https://github.com/ionut-slaveanu/hyper.git" }

The problem with hyper's graceful_shutdown function that I'm trying to fix in that PR is:

• if the graceful_shutdown function is called during the http2 handshake, no GOAWAY frame is sent by the server to the clients. Basically, clients don't know that the http2 channel has been closed. Because of this some responses are lost.

[tottoto]

By the way, as another approach, could this be solved in a more general way by adding an ability to specify a Future of signal that cancels the graceful shutdown? This allows the user to have options that are not limited to timeout.

[djc]

By the way, as another approach, could this be solved in a more general way by adding an ability to specify a Future of signal that cancels the graceful shutdown? This allows the user to have options that are not limited to timeout.

I'm not sure how cancelling graceful shutdown is involved? Seems to me this PR is about closing connections so that load balancers have a chance to spread the load differently.

[tottoto]

I misunderstood the process a little at the time. I think you are right. I consider whether there is any other useful Futures besides timeouts, but I could not find out, so if we add this functionality, a timeout seems like a good idea.

[djc]

Thanks, I think this makes sense.

[ionut-slaveanu]

Thanks, I think this makes sense.

Thank you, too!