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

Make quinn-proto::{Connection, Endpoint} deterministic #1691

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

michael-yxchen
Copy link
Contributor

quinn-proto::{Connection, Endpoint} structs call StdRng::from_entropy() in their implementation. It makes it hard to reproduce errors in an otherwise deterministic test case.

This PR addresses the issue by adding a StdRng argument to the respective new functions. For other functions that may create an endpoint/connection, an argument of type impl FnOnce() -> StdRng is added. This avoids eager calls to generate fresh entropy. e.g. the Endpoint::handle function is frequently called but we'd only need the rng when handling a new incoming connection.

Fixes an unrelated lint error.

Closes #1684

`quinn-proto::{Connection, Endpoint}` structs call
`StdRng::from_entropy()` in their implementation. It makes it hard to
reproduce errors in an otherwise deterministic test case.

This PR addresses the issue by adding a `StdRng` argument to the
respective `new` functions. For other functions that may create an
endpoint/connection, an argument of type `impl FnOnce() -> StdRng` is
added. This avoids eager calls to generate fresh entropy. e.g. the
`Endpoint::handle` function is frequently called but we'd only need the
rng when handling a new incoming connection.

Fixes lint errors.
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

I'd prefer to avoid making rand a public dependency if possible. Could we accept a seed instead of an RNG?

quinn-proto/src/endpoint.rs Outdated Show resolved Hide resolved
@michael-yxchen
Copy link
Contributor Author

Put up another version that always use Endpoint.rng to seed Connection.rng. Let me know if you like it this way, or prefer to obtain fresh randomness from the entropy pool if Endpoint.rng is not seeded

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks, that looks cleaner!

quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
quinn-proto/src/endpoint.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

@djc djc merged commit dd34d57 into quinn-rs:main Oct 23, 2023
7 of 8 checks passed
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.

quinn-proto: Non-determinism in endpoint/connection/congestion
3 participants