Improve FakeTimeProvider into a canonical fake

[danmoseley]

The one in this repo has some issues/limitations,

1. Consider allowing an arbitrary TimespanFrequency.

Currently it is set to 10^7 == TimeSpan.TicksPerSecond. The actual TimeProvider.System uses TimespanFrequency == Stopwatch.Frequency == typically 10^6 or 10^9 (and does not change once the system is started). A different TimespanFrequency could help spot bugs where code assumes TimeSpan.TicksPerSecond.

Note that while GetElapsedTime() abstracts Frequency, Kestrel generally does math like Expiration = currentTimestamp + timeoutTimeSpan.TotalSeconds * timeProvider.TimespanFrequency, and then later if (currentTimestamp > Expiration) ... . See here for some examples

If TimespanFrequency changes we would need to take care about truncation. We don't do math here in order to keep stable test results - essentially long has more digits than double. GetElapsedTime() is not virtual, so can't be fixed; one option is possibly to ensure that the base ticks are rounded up front to the nearest 1ms or so.

2. Consider allowing an arbitrary base offset. Eg., one of the Kestrel ones randomizes the base offset. Many of the tests in this repo use UtcNow, which actually adds non reproducibility, example.

3. Look at the TimeProvider's in dotnet/aspnetcore and dotnet/runtime possibly elsewhere, and consider whether we can make one with the combined features that we can share via dotnet/arcade instead of each having our own. Ideally we have a single test TimeProvider that can be configured however needed. Right now we have numerous ones.

[danmoseley]

Here's some extension methods they used in Kestrel

    public static long ToTicks(this TimeSpan timeSpan, TimeProvider timeProvider)
        => timeSpan.ToTicks(timeProvider.TimestampFrequency);

    public static long ToTicks(this TimeSpan timeSpan, long tickFrequency)
    {
        if (timeSpan == TimeSpan.MaxValue)
        {
            return long.MaxValue;
        }
        if (timeSpan == TimeSpan.MinValue)
        {
            return long.MinValue;
        }
        return timeSpan.Ticks * (tickFrequency / TimeSpan.TicksPerSecond);
    }

    public static long GetTimestamp(this TimeProvider timeProvider, TimeSpan timeSpan)
    {
        return timeProvider.GetTimestamp(timeProvider.GetTimestamp(), timeSpan);
    }

    public static long GetTimestamp(this TimeProvider timeProvider, long timeStamp, TimeSpan timeSpan)
    {
        return timeStamp + timeSpan.ToTicks(timeProvider);
    }

are any of these useful to add here, or to the base TimeProvider? Note, some will have the truncation problem.

[egil]

GetElapsedTime() is not virtual...

TimeProvider is not released yet, so it should be possible to make it virtual. This seems like a very good reason to do so.

I am also pushing for other changes that have a positive impact on testing: dotnet/runtime#85326

[danmoseley]

TimeProvider is not released yet, so it should be possible to make it virtual. This seems like a very good reason to do so.

I agree, it's possible. Feel free to open an issue.

cc @tarekgh

[tarekgh]

I am not seeing making GetElapsedTime virtual is right thing. GetElapsedTime works on the snapped time by the provider and measure the time according to the time provider frequency. Maybe it is better to expose some helper to convert from/to timespan frequency like the extension methods mentioned in the comment #3996 (comment).

CC @stephentoub

[egil]

I generally think it is important that the FakeTimeProvider behaves exactly like the real timer, besides actually having time pass.

Another example where the current implementation (and my own) fail is how timer callbacks are invoked. Consider this test:

// This fails since the Change call blocks until the callback completes
[Fact]
public void Async_timer_callback_with_duetime_zero()
{
    // Arrange
    var sut = new FakeTimeProvider();
    var callbacks = new List<DateTimeOffset>();

    // Act
    using var timer = sut.CreateTimer(
        _ =>
        {
            Thread.Sleep(20);
            callbacks.Add(sut.GetUtcNow());
        },
        null,
        TimeSpan.Zero,
        Timeout.InfiniteTimeSpan);
    timer.Change(TimeSpan.Zero, Timeout.InfiniteTimeSpan);

    // Assert
    callbacks.Should().BeEmpty();
}

// This succeeds since the Change call does not block until the callback completes
[Fact]
public void Async_timer_callback_with_duetime_zero()
{
    // Arrange
    var sut = TimeProvider.System;
    var callbacks = new List<DateTimeOffset>();

    // Act
    using var timer = sut.CreateTimer(
        _ =>
        {
            Thread.Sleep(20);
            callbacks.Add(sut.GetUtcNow());
        },
        null,
        TimeSpan.Zero,
        Timeout.InfiniteTimeSpan);
    timer.Change(TimeSpan.Zero, Timeout.InfiniteTimeSpan);

    // Assert
    callbacks.Should().BeEmpty();
}

This particular issue bid me today in a scenario where timer.Change() was called while inside a lock, which the callback tries to enter, leading to a deadlock.

[geeknoid]

The FakeTimeProvider in this repo is the one we're shipping to customers. It has a robust feature set, so I'm closing this for now. We can create new issues when specific customer requests come in.