fix: preserve Unlimited StreamsInbound in connmgr reconciliation #9723
Conversation
|
@Jorropo : do we have any tests around this? |
core/node/libp2p/rcmgr_defaults.go
Outdated
| @@ -129,7 +129,9 @@ func createDefaultLimitConfig(cfg config.SwarmConfig) (limitConfig rcmgr.Concret | |||
| } | |||
|
|
|||
| // Scale System.StreamsInbound as well, but use the existing ratio of StreamsInbound to ConnsInbound | |||
| partialLimits.System.StreamsInbound = rcmgr.LimitVal(maxInboundConns * int64(partialLimits.System.StreamsInbound) / int64(partialLimits.System.ConnsInbound)) | |||
| if partialLimits.System.StreamsInbound != rcmgr.Unlimited { | |||
There was a problem hiding this comment.
Would it make sense to change this check to if partialLimits.System.StreamsInbound >= 0 to protect against all enums (including any future ones that are added)?
There was a problem hiding this comment.
That will silently ignore new enums.
There was a problem hiding this comment.
that's the point, we don't ever want to include enums in this calculation right?
There was a problem hiding this comment.
I don't know, maybe there is a new enum in the future that will require some new logic.
What I would want is some Rust like enum matching, where if a new enum is added the build would fail here.
There was a problem hiding this comment.
The same issue would occur in any language if you overload an int w/ multiple semantics depending on where a value lies in an interval.
This depends on the register width and is pretty deep from the config. |
|
@Jorropo : my thinking was more that if we set |
|
@BigLep I see, I made that, I thought you meant to test the underflow. @guseggert see the latest commit, I think it make the code worst. |
There was a problem hiding this comment.
Code seems good/right to me. I added a suggestion about driving people to https://github.com/ipfs/kubo/blob/master/docs/libp2p-resource-management.md#how-does-the-resource-manager-resourcemgr-relate-to-the-connection-manager-connmgr
|
Unrelated known flaky failures. |
Fixes #9695