-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added WithConnLimiter option for resource manager #1
base: master
Are you sure you want to change the base?
Conversation
@@ -99,7 +99,7 @@ Some notable users of go-libp2p are: | |||
- [Status go](https://github.com/status-im/status-go) - Status bindings for go-ethereum, built by [Status.im](https://status.im/) | |||
- [Flow](https://github.com/onflow/flow-go) - A blockchain built to support games, apps, and digital assets built by [Dapper Labs](https://www.dapperlabs.com/) | |||
- [Swarm Bee](https://github.com/ethersphere/bee) - A client for connecting to the [Swarm network](https://www.ethswarm.org/) | |||
- [Elrond Go](https://github.com/multiversx/mx-chain-go) - The Go implementation of the the Elrond network protocol | |||
- [MultiversX Node](https://github.com/multiversx/mx-chain-go) - The Go implementation of the MultiversX network protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -74,11 +74,16 @@ func sortNetworkPrefixes(limits []NetworkPrefixLimit) []NetworkPrefixLimit { | |||
// for a specific subnet than the default limit per subnet. | |||
func WithNetworkPrefixLimit(ipv4 []NetworkPrefixLimit, ipv6 []NetworkPrefixLimit) Option { | |||
return func(rm *resourceManager) error { | |||
connLimiterInstance, ok := rm.connLimiter.(*connLimiter) | |||
if !ok { | |||
return nil // early exit, it might be a custom implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe throw an error here? Just to signal that the method call did not produce the expected result. Valid also on L100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
if ipv4 != nil { | ||
rm.connLimiter.connLimitPerSubnetV4 = ipv4 | ||
connLimiterInstance.connLimitPerSubnetV4 = ipv4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to protect this set method with the same connLimiter.mu mutex. Maybe extract these set methods in the implementation + interface and get rid of the casts?
Considering the case where multiple hosts are running on the same ip, it would be very hard to decide what would be the proper limit in terms of using one. Although the default options, with ConnCount set to math.MaxInt would be fix this problem, the internal array of maps would still consume a lot of memory.
WithConnLimiter option would be a great option in order to allow clients their own custom implementation of the connLimiter. In our case, simply use an empty limiter would be the best option for the moment.
Also added a tiny change request on the readme, in order to completely align with our new naming.