Using new TimeProvider #2108
Using new TimeProvider #2108
Conversation
|
Note to self, offline feedback from Miha.
|
| try | ||
| { | ||
| _timer.Change(_period, Timeout.Infinite); | ||
| _timer.Change(_period, Timeout.InfiniteTimeSpan); |
There was a problem hiding this comment.
Is this expected that the variable is named _period but it's used on the dueTime argument?
So it will be executed once, delayed, and not periodically.
public bool Change (int dueTime, int period);
There was a problem hiding this comment.
It re-starts itself after running the current callback.
https://github.com/microsoft/reverse-proxy/blob/4868c5529d69ed55ef7a7cd101a3a7ae183583e4/src/ReverseProxy/Health/EntityActionScheduler.cs#L238
| timeProvider.VerifyTimer(1, Period1); | ||
|
|
||
| timerFactory.FireTimer(1); | ||
| timeProvider.FireTimer(1); |
There was a problem hiding this comment.
This PR has nothing to do with this, but I have to say that I find it a weird way to handle fake time. I would just expect the timer to call Advance and have the callbacks be triggers automatically, not the test to explicitly invoke the callback.
There was a problem hiding this comment.
Do we know when the built-in test time provider will ship, such that we can remove this one?
sebastienros
left a comment
sebastienros
left a comment
There was a problem hiding this comment.
Left two comments but I assume you will have an answer and nothing to change
|
|
||
| public int TimerCount => _timers.Count; | ||
|
|
||
| public override long TimestampFrequency => TimeSpan.TicksPerSecond; |
There was a problem hiding this comment.
| public override long TimestampFrequency => TimeSpan.TicksPerSecond; | |
| public override long TimestampFrequency => TimeSpan.TicksPerSecond * 7; |
Nit: Can we have this return a non-standard value, so that we can catch product errors that our tests would be happy with (e.g. we offset a timestamp by timeSpan.Ticks somewhere instead of going through TimeProvider)?
TimestampFrequency and GetTimestamp are the only places that should have to change.
|
|
||
| public override DateTimeOffset GetUtcNow() => new DateTime(_currentTime.Ticks, DateTimeKind.Utc); | ||
|
|
||
| public override long GetTimestamp() => _currentTime.Ticks; |
There was a problem hiding this comment.
| public override long GetTimestamp() => _currentTime.Ticks; | |
| public override long GetTimestamp() => _currentTime.Ticks * 7; |
Fixes #2084
IClock was the only public abstraction type, it's been marked obsolete. It also wasn't consumed publicly anywhere, only internally via DI. All other internal abstractions have been removed. The only people potentially broken by this change are those injecting IClock into their own code or trying to inject their own IClock into our components for some reason.
The K8 code was using ISystemClock from Microsoft.Extensions. There are breaking public API changes there to replace it.
The SDK update broke some asserts in the telemetry consumers that I've commented out. Miha will fix these later.