Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to keepalive-connection shedding #2628

Merged
merged 3 commits into from May 20, 2021

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented May 12, 2021

Description

As a followup to df72887, this PR is an attempt to refactor the 'keepalive-connection shedding' logic to improve its performance and reliability.

Summary of changes:

  1. Instead of closing the socket by breaking out of the Server#process_client loop from the 'keepalive == true' branch, move the connection-shedding logic to modify the res_info[:keep_alive] variable in Request#str_headers. This writes the Connection: close header as part of the final response before closing the socket, allowing clients to reopen a new connection more gracefully (for example, preventing 'socket read' errors from being logged in wrk).

  2. Instead of shedding the keepalive connection after a fixed number of consecutive inline requests, shed the connection whenever the server is 'at capacity' (busy threads >= max), and there is a new pending connection in the listener socket waiting to be accepted.

  3. Additionally, push a keepalive connection back to the reactor whenever there's a pending request in the threadpool backlog (but not a new connection pending), to ensure that requests are serviced fairly across all the keepalive connections already accepted in the server.

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.

@nateberkopec
Copy link
Member

LGTM and fixes what I saw in wrk and hey. Should release this soon.

@nateberkopec nateberkopec merged commit ffa5d56 into puma:master May 20, 2021
@nateberkopec
Copy link
Member

I'm gonna backport to 4.3 as well

@ducharmemp
Copy link

👋 Hey @nateberkopec was this ever backported to 4.3? I checked the release notes and didn't see a reference to this fix

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
* Improvements to keepalive-connection shedding

* alternative still using max_fast_inline

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug perf waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keepalive connections which exceed request limit closed in a less-than-clean manner with 5.3.1
4 participants