-
Notifications
You must be signed in to change notification settings - Fork 286
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
balance: Remove MakeBalance::from_rng #497
Conversation
`MakeBalance`'s use of `SmallRng` is problematic: since it clones the `SmallRng` between `Balance` instances, each instance will have the same state and produce an identical sequence of values. This probably isn't _dangerous_, though it is certainly unexpected. This change removes the `MakeBalance::from_rng` and `MakeBalanceLayer::from_rng` helpers. The `MakeBalance` service now uses the default RNG via `Balance::new`. `Balance::new` now creates its `SmallRng` from the `thread_rng` instead of the default entropy source, as the default entropy source may use the slower `getrandom`. From the [`rand` docs][from_entropy]: > In case the overhead of using getrandom to seed many PRNGs is an > issue, one may prefer to seed from a local PRNG, e.g. > from_rng(thread_rng()).unwrap(). Finally, this change updates the balnancer to the most recent version of `rand`, v0.8.0. [from_entropy]: https://docs.rs/rand/0.8.0/rand/trait.SeedableRng.html#method.from_entropy
Looks like the examples need to be updated. |
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.
This seems good to me once the examples compile again!
It does seem like it could be possible to allow constructing MakeBalance
instances with an overridden RNG, perhaps by adding a constructor that takes an FnMut() -> Rng
? That way, the RNG is overridable at the MakeService
level as well as the Service
level, but with a way of making a new RNG with its own state. But, we can add that in a follow-up if we decide it's even worth having. In practice, I'm not really sure if there's all that much of a need for this --- maybe if you want to mock out randomness for deterministic testing? I dunno.
@hawkw We still support |
that's fair, this is fine! |
MakeBalance
's use ofSmallRng
is problematic: since it clones theSmallRng
betweenBalance
instances, each instance will have the samestate and produce an identical sequence of values. This probably isn't
dangerous, though it is certainly unexpected.
This change removes the
MakeBalance::from_rng
andMakeBalanceLayer::from_rng
helpers. TheMakeBalance
service now usesthe default RNG via
Balance::new
.Balance::new
now creates itsSmallRng
from thethread_rng
instead of the default entropy source,as the default entropy source may use the slower
getrandom
. From therand
docs:Finally, this change updates the balancer to the most recent version of
rand
, v0.8.0.This seems like a change we should make before the next release/version
bump of
tower
.