FakeTimeProvider.Advance/SetUtcNow does not behave as expected

[egil]

The current implementation of FakeTimeProviders Advance(TimeSpan) and SetUtcNow(DateTimeOffset) does not behave as expected.

There are a few different scenarios, so let me take them one at a time:

Scenario 1: A timer with dueTime = 5 second and period = 5 seconds.

Calling Advance(TimeSpan.FromSeconds(5)) at time 0 should invoke the timer callback 1 time (works).
Calling Advance(TimeSpan.FromSeconds(10)) at time 0 should invoke the timer callback 2 time (does not work).

Here are two tests that cover this:

[Fact]
public void Timer_callback_invoked_multiple_times_single_advance()
{
   var sut = new FakeTimeProvider();
   var callbackCount = 0;
   var dueTime = TimeSpan.FromSeconds(3);
   var period = TimeSpan.FromSeconds(5);
   using var timer = sut.CreateTimer(_ => callbackCount++, null, dueTime, period);

   sut.Advance(TimeSpan.FromSeconds(13));

   callbackCount.Should().Be(3);
}

[Fact]
public void GetUtcNow_matches_time_at_callback_time()
{
   var sut = new FakeTimeProvider();
   var startTime = sut.GetUtcNow();
   var callbackTimes = new List<DateTimeOffset>();
   var interval = TimeSpan.FromSeconds(3);
   using var timer = sut.CreateTimer(_ => callbackTimes.Add(sut.GetUtcNow()), null, interval, interval);

   sut.Advance(interval + interval + interval);

   callbackTimes.Should().ContainInOrder(
       startTime + interval,
       startTime + interval + interval,
       startTime + interval + interval + interval);
}

Scenario 2: A periodic timer with a period = 10 seconds and a delay of 3 seconds inserted after each WaitForNextTickAsync completes.

The point of this scenario is that during a timer callback, another thing can be scheduled.

Calling Advance(TimeSpan.FromSeconds(23)) at time 0 should result in callback invocations at times 10, 13, 20, and 23.

Here is a test that covers this:

[Fact]
public async Task Callbacks_happens_in_schedule_order()
{
    var sut = new FakeTimeProvider();
    var periodicTimer = sut.CreatePeriodicTimer(TimeSpan.FromSeconds(10));
    var startTime = sut.GetUtcNow();
    var callbacks = new List<DateTimeOffset>();
    var callbacksTask = AsyncCallbacks(periodicTimer);

    sut.Advance(TimeSpan.FromSeconds(23));

    callbacks.Should().ContainInOrder(
        startTime + TimeSpan.FromSeconds(10),
        startTime + TimeSpan.FromSeconds(13),
        startTime + TimeSpan.FromSeconds(20),
        startTime + TimeSpan.FromSeconds(23));

    periodicTimer.Dispose();
    await callbacksTask;

    async Task AsyncCallbacks(PeriodicTimer periodicTimer)
    {
        while (await periodicTimer.WaitForNextTickAsync().ConfigureAwait(false))
        {
            callbacks.Add(sut.GetUtcNow());
            await sut.Delay(TimeSpan.FromSeconds(3));
            callbacks.Add(sut.GetUtcNow());
        }
    }
}

I could see the benefit in a SkipAhead/BendSpaceAndTime or similar method, that allows the test user to skip forward in time from t0 to tN and explicitly not invoke any callbacks between t0 and tN, and only invoke callbacks set to trigger at tN. However, while using my own version of FakeTimeProvider in production for the last six months I have yet to have a need. I have however needed both of the scenarios described above in my testing.

Ps. If you want to compare notes, here is my implementation, ManualTimeProvider.

[tarekgh]

CC @geeknoid @joperezr @sebastienros

[sebastienros]

That's actually how I thought it should have worked initially, but AFAIR the existing implementation and tests were not behaving this way. I'll talk with internal stakeholders and comment back here.

[egil]

That's actually how I thought it should have worked initially, but AFAIR the existing implementation and tests were not behaving this way. I'll talk with internal stakeholders and comment back here.

The current implementation and related tests actively do the "SkipAhead/BendSpaceAndTime", i.e., invoke whatever times are scheduled at the time Advance or SetUtcNow is called. It does not include new timer callbacks being added during a timer callback.

My own implementation did initially as well, so I can understand why the current implementation looks like it does.

[danmoseley]

Linking #3996 here as well. It would be good to look around at other fake TimeProvider's and include the best of all.

[sebastienros]

I investigated and I am agreeing with your suggestion. I will propose the changes soon unless you beat me to it. (not before next week)

[danmoseley]

do you want to throw up a PR @egil?

[egil]

@danmoseley I can probably find some time in the coming days to do so. It should not be too much work if I can base the solution on my https://github.com/egil/TimeProviderExtensions/blob/main/src/TimeProviderExtensions/ManualTimeProvider.cs. If you want to stick with your current solution with adjustments to make it behave correctly, I will probably need some more time, and @sebastienros be the better option.

Let me know what you prefer!

[egil]

In addition, there is another concern mentioned in #3996 (comment) that may be related and could be part of a PR, or it could be its own issue and PR. Probably the latter.

[danmoseley]

@sebastienros can answer your first question (I have no opinion -- just wanted to make sure you got the chance to do it if you wanted)

for the second, I'd suggest a separate PR.

BTW, I do appreciate you're looking at all these fake time providers. As noted in that issue, I hope we can converge these a bit.

[egil]

BTW, I do appreciate you're looking at all these fake time providers. As noted in that issue, I hope we can converge these a bit.

Likewise. It's an interesting problem, and I need one too :)

I do think the entire motivation for creating the TimeProvider was to give the ecosystem a single abstraction and ideally, also a single "fake" version that we could all lean on instead of all creating our own.

[geeknoid]

BTW, I do appreciate you're looking at all these fake time providers. As noted in that issue, I hope we can converge these a bit.

Likewise. It's an interesting problem, and I need one too :)

I do think the entire motivation for creating the TimeProvider was to give the ecosystem a single abstraction and ideally, also a single "fake" version that we could all lean on instead of all creating our own.

The motivation for TimeProvider/FakeTimeProvider came from needing to write tests for all the code in this repo. Then we started to look around and found literally hundreds of variations of IClock types within Microsoft services. It became very clear that this needed a universal abstraction.

[danmoseley]

If we can make a fake useful for the 90% case, it's not clear where we'd ship it since code lower in the stack would use it. @joperezr is it legit for runtime and aspnetcore to depend on a test package out of this repo?

[egil]

The motivation for TimeProvider/FakeTimeProvider came from needing to write tests for all the code in this repo. Then we started to look around and found literally hundreds of variations of IClock types within Microsoft services. It became very clear that this needed a universal abstraction.

That is my understanding too, and what I attempted to write.

If we can make a fake useful for the 90% case, it's not clear where we'd ship it since code lower in the stack would use it. @joperezr is it legit for runtime and aspnetcore to depend on a test package out of this repo?

After reading the original issue on TimeProvider I was under the impression that there would be an official ManualTimeProvider/FakeTimeProvider released from the "dotnet team".

Are you expecting that multiple teams will build their own and release them independently or just keep them internal in the individual repos?

[danmoseley]

Yes I'm hoping we can ship a standard fake. Just wondering where it would go so we ourselves could use it. We don't have such things at the bottom of the stack, but maybe should. Or we ship from this level, and copy source lower down.

[egil]

Yes I'm hoping we can ship a standard fake. Just wondering where it would go so we ourselves could use it. We don't have such things at the bottom of the stack, but maybe should. Or we ship from this level, and copy source lower down.

Ahh, I see. Does "bottom of the stack" refer to the core libraries (System where TimeProvider resides) vs. at the "extension" level (optional libraries)?

[danmoseley]

Yep. I wouldn't think about that part, it's soluble.

[egil]

Just an FYI: I'll start working on a PR for this tonight.

[egil]

Cool. Thanks for the patience and all the feedback on the PR.

When will there be a NuGET released?

[RussKie]

Generally, a NuGet is published within few hours of a merge. Though the dev packages get published to https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet8 feed.

[egil]

Generally, a NuGet is published within few hours of a merge. Though the dev packages get published to https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet8 feed.

Good to know, thanks.

When is a nuget.org release planned?

[joperezr]

These packages follow the same release cycle than the rest of .NET platform, meaning the next time a preview package will be pushed to NuGet is preview 5. This change made it into the 8.0 preview 5 branch which is going to ship sometime next week, so you should expect a preview 5 package with these changes in NuGet then 😄