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

Remove oxy connections limiter #46900

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Remove oxy connections limiter #46900

merged 1 commit into from
Sep 30, 2024

Conversation

zmb3
Copy link
Collaborator

@zmb3 zmb3 commented Sep 24, 2024

lib/limiter composes a maximum simultaneous connections limiter as well as a rate limiter. This commit replaces the connections limiter from oxy with built-in code.

Note: this also introduces a bug fix that may change behavior. Prior to this change, the connection limiter kept a separate connection account for HTTP connections than it did for connections managed manually with acquire/release. We no longer maintain separate counts - all connections (HTTP or not) contribute to the number of allowed connections.

@zmb3 zmb3 marked this pull request as ready for review September 25, 2024 22:36
Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

lib/limiter/connlimiter.go Outdated Show resolved Hide resolved
lib/limiter/connlimiter.go Show resolved Hide resolved

// Wrap wraps an HTTP handler.
func (l *ConnectionsLimiter) Wrap(h http.Handler) {
l.next = h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this approach, a second call to Wrap is going to overwrite the first one and we might do that by accident, we could return a http.Handler from Wrap instead (with an inline http.HandlerFunc maybe):

func (l *ConnectionsLimiter) Wrap(next http.Handler) http.Handler {
  return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
    token, _, err := net.SplitHostPort(r.RemoteAddr)
    ...
    if err := l.AcquireConnection(token); err != nil {
      ...
    }
    defer l.ReleaseConnection(token)
    next.ServeHTTP(w, r)
  })
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love it either, it's just how this code already worked.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

Copy link

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

@zmb3 zmb3 added the no-changelog Indicates that a PR does not require a changelog entry label Sep 30, 2024
@zmb3 zmb3 enabled auto-merge September 30, 2024 21:38
lib/limiter composes a maximum simultaneous connections limiter
as well as a rate limiter. This commit replaces the connections
limiter from oxy with built-in code.

Note: this also introduces a bug fix that may change behavior.
Prior to this change, the connection limiter kept a separate
connection account for HTTP connections than it did for connections
managed manually with acquire/release. We no longer maintain separate
counts - all connections (HTTP or not) contribute to the number
of allowed connections.
@zmb3 zmb3 added this pull request to the merge queue Sep 30, 2024
Merged via the queue into master with commit 7ff42d0 Sep 30, 2024
39 checks passed
@zmb3 zmb3 deleted the zmb3/remove-oxy branch September 30, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants