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

[core/swarm] Refactor and extend configurable connection limits. #1848

Merged
merged 6 commits into from
Nov 23, 2020

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Nov 20, 2020

The initial motivation for the work in this PR is #1839. However, the PR goes a bit further than to just add a single counter to have a more streamlined API. Thus, in order to better track different connection counts and permit configurable limits for these counts and make these available for inspection in an efficient manner (i.e. without linear counting operations), I introduced dedicated connection counters via a ConnectionCounters structure maintained by the internal connection pool but exposed on the API via the NetworkInfo. All connection states that are counted in this way can also have effective configurable limits.

All limits are now configured through a ConnectionLimits structure. The new configurable limits are

  • ConnectionLimits::max_established_incoming for limiting the number of concurrent established connections that were incoming.
  • ConnectionLimits::max_established_outgoing for limiting the number of concurrent established connections that were outgoing.

A limit that has been removed because tracking the related counts efficiently would be a bit laborious and I don't think it is particularly useful is

  • NetworkConfig::set_outgoing_per_peer_limit()

Note that there are still the pre-existing ConnectionLimits::max_incoming and ConnectionLimits::max_outgoing which correspond to the two new configuration options above, just for pending connections. I also took the liberty to change the connection limits to use u32 instead of usize for their types, which seems more appropriate.

Fixed panic in the Peer API

The Network::peer() API allows to iterate mutably over the ongoing connection attempts via DialingPeer::attempts(), where each DialingAttempt can be abort()ed during iteration. However, the iterator did not take into account the changes to the internal iteration offsets when elements are removed. This was discovered while extending the connection limit tests a bit and has been fixed now as a result.

Roman S. Borschel added 2 commits November 20, 2020 13:27
To better track different connection counts, permit configurable
limits for these counts and make these available for
inspection efficiently, introduce dedicated connection counters
via a `ConnectionCounters` structure that is exposed on the
API via the `NetworkInfo`. All connection or connection
states that are counted in this way can also have effective
configurable limits.
@romanb romanb linked an issue Nov 20, 2020 that may be closed by this pull request
@romanb romanb changed the title [core] Refactor and extend configurable connection limits. [core/swarm] Refactor and extend configurable connection limits. Nov 20, 2020
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Good catch on the iterator state mismatch!

core/src/connection/pool.rs Outdated Show resolved Hide resolved
core/src/connection/pool.rs Outdated Show resolved Hide resolved
Copy link
Member

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Looks good to me with the same remarks as Max on the naming.

@romanb
Copy link
Contributor Author

romanb commented Nov 23, 2020

I incorporated the naming suggestion(s) in 4e447e9 together with a bit of refinement for the configuration APIs:

  • ConnectionLimits now has private fields for future extensions and offers a builder API that moves self.
  • NetworkConfig now has a builder API that moves self.

The SwarmBuilder already used this style, so there is some gain in consistency. Let me know if that last commit looks acceptable.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Both the connection limiting as well as the general API improvements look good to me.

core/CHANGELOG.md Show resolved Hide resolved
core/CHANGELOG.md Show resolved Hide resolved
romanb and others added 2 commits November 23, 2020 17:07
Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Max Inden <mail@max-inden.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to limit the number of total inbound connections
3 participants