Add opt to enable keep-alives

[slizco]

Description

Puma does not currently support a configuration option for enabling/disabling keep-alive connections to the server. Because a thread can only handle a single connection at a time, the Puma server under default config will end up serving requests out of order, preferring to process requests off it's already in flight connection. See #3487 for more details.

To get around this issue, Puma will close connections every max_fast_inline requests. When max_fast_inline=1, the server should close the connection after each request, but this only happens most of the time. It does not happen all the time, due to the following conditional:

puma/lib/puma/request.rb

Lines 163 to 169 in 48b89b5

	# Close the connection after a reasonable number of inline requests
	# if the server is at capacity and the listener has a new connection ready.
	# This allows Puma to service connections fairly when the number
	# of concurrent connections exceeds the size of the threadpool.
	force_keep_alive = requests < @max_fast_inline ||
	@thread_pool.busy_threads < @max_threads ||
	!client.listener.to_io.wait_readable(0)

Thus it's still possible to end up with long tail latencies because a small percentage of requests will sit in the queue for awhile.

Sometimes, the number of busy threads is less than the max and sometimes, there are no new connections to accept on the socket. However, these decisions are made at a point in time and the state of the server is constantly changing. They are subject to race conditions since other threads are concurrently accessing these variables and taking actions that modify their values.

The absolute safest way to ensure a connection is handed back to the reactor class once the request has been handled is to outright disable keep-alives. This PR proposes exposing an enable_keep_alives option which defaults to true, as to least disrupt the current behavior. Disabling keep-alives may be done by setting enable_keep_alives: false in the Puma config.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[schneems]

Thank you for the PR. I like the feature and think we should have some way to disable this behavior consistently. I have two requested and one optional change.

Config set to enable instead of disable.

Short:

• Request: Please invert the logic to enable_keep_alives
• Request: Please add the default value to the docs above the method as well

Long:

I'm thinking of how users will read the value when they're looking at their config/puma.rb without docs at hand and what they think it will do. The current logic for calling this code:

disable_keep_alives

That code would set @disable_keep_alives=falsewhich seems backwards. I think that's a small issue of flipping the default argument, however...

I'm also looking at other config examples like

puma/lib/puma/dsl.rb

Line 472 in 9ef6f83

def quiet(which=true)

that are boolean. Many use the argument of which and are framed as enabling some behavior. Such as drain_on_shutdown or clean_thread_locals. I'm also thinking of how this might be viewed from a user and I like representing the state positively instead of negatively:

enable_keep_alives
enable_keep_alives false

To me, the first line would enable keep alives (already the default) while the second would disable it. Comparing to

disable_keep_alives
disable_keep_alives false

The first line I would expect to disable the keepalives, but the next line presents a double negative that's a bit hard for my brain to parse. It also has the side benefit of being able to use it directly in the conditional without &&= !<variable>.

Code style

Short: Optionally update the code style to the if/else example below

Long:

I played with the style a bit and I found the &&= not entirely intuitive. It's used in other places in the file, but is not common. I tried this, but didn't love it:

 force_keep_alive = @enable_keep_alives && (
   requests < @max_fast_inline || 
   @thread_pool.busy_threads < @max_threads || 
   !client.listener.to_io.wait_readable(0)
 )

That code was tight, but hard to parse. I think this is more verbose, but clearer

force_keep_alive = if @enable_keep_alives
  requests < @max_fast_inline ||
    @thread_pool.busy_threads < @max_threads ||
    !client.listener.to_io.wait_readable(0)
else
  false
end

Another option is:

 force_keep_alive = @enable_keep_alives
 force_keep_alive &&= 
   requests < @max_fast_inline || 
   @thread_pool.busy_threads < @max_threads || 
   !client.listener.to_io.wait_readable(0)

My main worry is that someone might miss the trailing &&= in the existing PR when reading. It's a bit of a bike-shed though. I don't love any of them, but the if/else is my least bad pick. Especially if we start adding in comments, it could increase the distance between the effects. I also want to update the comments that describe the code, but that should be in a new PR and not cloud up this one.

[slizco]

@schneems I've updated this according to your suggestions. Let know your thoughts!

[schneems]

Overall looks great. I made some suggestions to comments/docs I would like to apply. I'm approving now and will apply those changes and merge once the suite passes.

Thanks a ton for working on this!

[dentarg]

Some lint fixes needed: https://github.com/puma/puma/actions/runs/11038443413/job/30661661652?pr=3496

[schneems]

@dentarg thanks. I fixed up the trailing whitespace, now I'm seeing failures in CI

Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:692973e3d937129bcbf40652eb9f2f61becf3332)
Download action repository 'ruby/setup-ruby@v1' (SHA:f3[21](https://github.com/puma/puma/actions/runs/11039500154/job/30665340740#step:1:25)cf5a4d1533575411f8752cf25b86478b0442)
Warning: Failed to download action 'https://internal-api.service.iad.github.net/repos/ruby/setup-ruby/tarball/f321cf5a4d1533575411f8752cf25b86478b0442'. Error: Name or service not known (internal-api.service.iad.github.net:443)
Warning: Back off 12.968 seconds before retry.
Warning: Failed to download action 'https://internal-api.service.iad.github.net/repos/ruby/setup-ruby/tarball/f321cf5a4d1533575411f8752cf25b86478b0442'. Error: Name or service not known (internal-api.service.iad.github.net:443)
Warning: Back off 18.971 seconds before retry.
Error: Name or service not known (internal-api.service.iad.github.net:443)

Not seeing anything on GitHub's status site

[schneems]

Third time was the charm. CI passed, merging.

[slizco]

Thanks @schneems and @dentarg!