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

Using new TimeProvider #2108

Merged
merged 9 commits into from Apr 26, 2023
Merged

Using new TimeProvider #2108

merged 9 commits into from Apr 26, 2023

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Apr 20, 2023

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.

@Tratcher Tratcher added this to the YARP 2.x milestone Apr 20, 2023
@Tratcher Tratcher self-assigned this Apr 20, 2023
src/ReverseProxy/Forwarder/StreamCopier.cs Outdated Show resolved Hide resolved
src/ReverseProxy/Forwarder/StreamCopier.cs Outdated Show resolved Hide resolved
src/ReverseProxy/Forwarder/StreamCopier.cs Outdated Show resolved Hide resolved
src/ReverseProxy/Forwarder/StreamCopier.cs Outdated Show resolved Hide resolved
src/ReverseProxy/Forwarder/StreamCopier.cs Outdated Show resolved Hide resolved
@Tratcher
Copy link
Member Author

Note to self, offline feedback from Miha.

I think the issue is in the test time provider.
You're returning TimeSpan ticks from GetTimestamp, but the frequency is still set to Stopwatch.Frequency.
You can either override TimestampFrequency and return TimeSpan.TicksPerSecond, or return (long)(_currentTime.TotalSeconds * TimestampFrequency) from GetTimestamp.
Or use some random frequency number that's different from TimeSpan/Stopwatch so that our tests don't accidentally work.
The frequency of Stopwatch on Unix is in nanoseconds, while TimeSpan ticks are 100-nanoseconds

@Tratcher Tratcher marked this pull request as ready for review April 25, 2023 17:49
@@ -183,7 +183,7 @@ private void SetTimer()

try
{
_timer.Change(_period, Timeout.Infinite);
_timer.Change(_period, Timeout.InfiniteTimeSpan);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

It re-starts itself after running the current callback.


timerFactory.FireTimer(1);
timeProvider.FireTimer(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Do we know when the built-in test time provider will ship, such that we can remove this one?

Copy link
Contributor

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

Left two comments but I assume you will have an answer and nothing to change

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!


public int TimerCount => _timers.Count;

public override long TimestampFrequency => TimeSpan.TicksPerSecond;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public override long GetTimestamp() => _currentTime.Ticks;
public override long GetTimestamp() => _currentTime.Ticks * 7;

@Tratcher Tratcher merged commit be58d84 into main Apr 26, 2023
2 of 6 checks passed
@Tratcher Tratcher deleted the tratcher/clocks branch April 26, 2023 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using new TimeProvider instead of ITimer and ITimerFactory
3 participants