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 SystemTime mockable via object #2089

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

gretchenfrage
Copy link
Contributor

@gretchenfrage gretchenfrage commented Dec 15, 2024

  • Adds trait SystemTimeClock
  • Adds default implementation RealSystemTimeClock
  • Adds ServerConfigParameter system_time_clock: Arc
  • Replaces all SystemTime::now calls in proto with SystemTimeClock.now calls (there were 2)

This is a backwards-compatible change.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

@aochagavia given that you've used Quinn in simulated time environments, any feedback on this?

quinn-proto/src/config/mod.rs Outdated Show resolved Hide resolved
@aochagavia
Copy link
Contributor

aochagavia commented Dec 16, 2024

@djc I haven't needed something like this at all, since as far as I know quinn fully supports mocking time through tokio. In fact, tokio's time mocking features are incredibly powerful, allowing you to automatically time-warp to the next event if the runtime isn't blocked on any other operation. I'd advice anyone needing clock mocking to look into tokio::time::pause and tokio::time::advance.

Update: I see now that this is about SystemTime, so the paragraph above doesn't totally apply (I was thinking of Instant). It looks like I might eventually need something like what this PR proposes, but I'm not fully sure yet.

@gretchenfrage
Copy link
Contributor Author

Update: I see now that this is about SystemTime, so the paragraph above doesn't totally apply (I was thinking of Instant). It looks like I might eventually need something like what this PR proposes, but I'm not fully sure yet.

Yeah, I believe proto already doesn't have any occurrences of Instant::now(), and that they're all passed in through method params. Possible I missed one though.

- Adds trait TimeSource
- Adds default implementation StdSystemTime
- Adds ServerConfigParameter time_source: Arc<dyn TimeSource>
- Replaces all SystemTime::now calls in proto with TimeSource.now calls
  (there were 2)

This is a backwards-compatible change.
@djc djc added this pull request to the merge queue Dec 17, 2024
@djc
Copy link
Member

djc commented Dec 17, 2024

Thanks!

Merged via the queue into quinn-rs:main with commit 51606c6 Dec 17, 2024
18 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.

4 participants