twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.78k stars 1.45k forks source link

`finagle-http2`: Add a per session request limit option #954

Closed beatrichartz closed 10 months ago

beatrichartz commented 1 year ago

finagle-http2: Add a per session request limit option

Problem

When running finagle in a setup using HTTP/2 via application load balancers (ALB), the session is very likely subject to a limit on the number of requests. E.g. when running via NGINX, the request limit is 1K requests per connection by default. On AWS, ALB have a 10k request limit. This means that in setups that use finagle and ALBs for their L7 features, the session is subject to races happening on connection close regularly. Here's an illustration of the race happening after a limit of 10K requests has been reached:

Illustration of race condition leading to Connection Closed Errors

Netty fires inactive channel for any request that is in flight for the closed session, leading Finagle to propagate a ChannelClosedException. There is no possible remediation for this race in Netty since Finagle manages sessions/connections.

This condition can be easily reproduced by running a Finagle server & client with an ALB like NGINX in between, sending concurrent requests up until the limit.

Solution

Allow configuration of a MaxRequestsPerSession option that aligns a session with underlying request limits. The number ofMaxConcurrentStreams is deducted to account for any requests in flight. When this number of requests is reached, mark the session as closed so it is shut down and a new session is created.

This option is off by default and therefore opt-in.

Result

When the configured number of requests is reached, the session is terminated and a new session is opened:

Illustration of session closing and reopening behavior using the per session request limit

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

mattdickinson5 commented 10 months ago

hmm, for some reason this landed attributing the wrong person - apologies!

beatrichartz commented 10 months ago

No worries, glad it landed! 👍