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

Add a control to limit connection reuses #678

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Apr 11, 2023

Motivation:

Sometimes it can be helpful to limit the number of times a connection
can be used before discarding it. AHC has no such support for this at
the moment.

Modifications:

  • Add a maximumUsesPerConnection configuration option which defaults
    to nil (i.e. no limit).
  • For HTTP1 we count down uses in the state machine and close the
    connection if it hits zero.
  • For HTTP2, each use maps to a stream so we count down remaining uses
    in the state machine which we combine with max concurrent streams to
    limit how many streams are available per connection. We also count
    remaining uses in the HTTP2 idle handler: we treat no remaining uses
    as receiving a GOAWAY frame and notify the pool which then drains the
    streams and replaces the connection.

Result:

Users can control how many times each connection can be used.

Motivation:

Sometimes it can be helpful to limit the number of times a connection
can be used before discarding it. AHC has no such support for this at
the moment.

Modifications:

- Add a `maximumUsesPerConnection` configuration option which defaults
  to `nil` (i.e. no limit).
- For HTTP1 we count down uses in the state machine and close the
  connection if it hits zero.
- For HTTP2, each use maps to a stream so we count down remaining uses
  in the state machine which we combine with max concurrent streams to
  limit how many streams are available per connection. We also count
  remaining uses in the HTTP2 idle handler: we treat no remaining uses
  as receiving a GOAWAY frame and notify the pool which then drains the
  streams and replaces the connection.

Result:

Users can control how many times each connection can be used.
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Apr 11, 2023
@glbrntt glbrntt requested a review from dnadoba April 11, 2023 09:59
usedStreams += count
precondition(usedStreams <= maxStreams, "tried to lease a connection which is not available")
self.state = .active(conn, maxStreams: maxStreams, usedStreams: usedStreams, lastIdle: lastIdle)
self.state = .active(conn, maxStreams: maxStreams, usedStreams: usedStreams, lastIdle: lastIdle, remainingUses: remainingUses.map { $0 - count })
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add a precondition for remainingUses >= count

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d5134bb

public var maximumUsesPerConnection: Int? {
willSet {
if let newValue = newValue {
precondition(newValue > 0, "maximumUsesPerConnection must be greater than zero or nil")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets make this a fatalError so this prints the reason in release builds as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in d5134bb

Copy link
Collaborator

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

LGTM minus two nits

@glbrntt glbrntt requested a review from dnadoba April 11, 2023 11:54
@glbrntt glbrntt merged commit 6c5058e into swift-server:main Apr 11, 2023
@glbrntt glbrntt deleted the gb-connection-use-limit branch April 11, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants