Changing FakeTimeProvider's current time during a timer callback can lead to lack of forward progress

[egil]

If a user has a timer with a duetime/period that is less than or equal to whatever AutoAdvanceAmount is set to, the fake time provider will go into an endless loop.

We discussed the pros/cons of having the feature in there already during the PR where AutoAdvanceAmount, but since the feature is going to stay this should be highlighted in the documentation.

For example, the following test will never reach the assertion statement.

[Fact]
public void Timer_callback_GetUtcNow_AutoAdvance()
{
    var oneSecond = TimeSpan.FromSeconds(1);
    var timeProvider = new FakeTimeProvider
    {
        AutoAdvanceAmount = oneSecond
    };

    var timer = timeProvider.CreateTimer(_ =>
    {
        timeProvider.GetUtcNow();
    }, null, TimeSpan.Zero, oneSecond);

    Assert.Fail("This is never reached.");
}

Ps. would still very much like to see examples where AutoAdvanceAmount is used to understand when it is useful.

[egil]

Alternatively, a guard can be added that prevents setting AutoAdvanceAmount to a number higher than any current timers callback period, and if AutoAdvanceAmount is set prior to a timer being created, a guard can prevent the timer from being created.

[geeknoid]

An alternate solution is to invoke the timer callback on a separate thread. This immediately fixes the example shown here, but it breaks many other FakeTimeProvider tests which were written to assume the callback would be run synchronously.

I'm not sure what the right thing to do is. Does running the callback asynchronously erode some of the value of FakeTimeProvider by reintroducing non-deterministic behavior? Or does it actually make it a better tool for testing since the overall behavior is closer to a real timer's in terms of calling environment.

Any thoughts?

[egil]

I am not sure, It depends on how the auto-advance feature should work.

Is this the desired behavior perhaps something like this?

    [Fact]
    public void Timer_callback_GetUtcNow_AutoAdvance()
    {
        var oneSecond = TimeSpan.FromSeconds(1);
        var timeProvider = new ManualTimeProvider() { AutoAdvanceAmount = oneSecond };

        var t1 = timeProvider.CreateTimer(_ =>
        {
            timeProvider.GetUtcNow();
        }, null, TimeSpan.Zero, oneSecond);

        Assert.Equal(timeProvider.Start + oneSecond, timeProvider.GetUtcNow());
    }

In general, though, I am not a fan of non-deterministic behavior if it can be avoided when it comes to fakes/mocks used with testing. That generally defeats the purpose and makes edge cases hard to debug for users.

[geeknoid]

That test wouldn't be reliable, since the timer callback could sometimes run before and sometimes run after the assert.

The behavior difference would not be limited to auto advance. If you just called the Advance function to move time forward, the timer callbacks of registered timers would be scheduled to execute on background threads. Yeah, I think this makes FakeTimeProvider generally less useful...

[egil]

Ohh, I didn't mean to run the test in a background thread. That also breaks a bunch of other things like just calling Advance and knowing that when the methods complete all scheduled callbacks have finished executing (unless they explicitly themselves spin up in a different thread), as you already mentioned.

My experimental implementation of a fake time provider (ManualTimeProvider) does not suffer from the endless loop problem though, and the example tests above pass with that, and with all other tests as well.

The difference between the two implementations seems to be that I am removing the "waiter" from the "Waiters" collection if it's due to having its callback invoked, and it is only added back in after the callback finishes running if needed. When the timer callback is what is causing the time to be (auto) advanced, then the next callback is scheduled after the SetUtcNow is finished, which prevents the loop.

The implementation is here if you are curious: https://github.com/egil/TimeProviderExtensions/blob/main/src/TimeProviderExtensions/ManualTimeProvider.cs

[geeknoid]

I'll try applying a similar technique. For the record though, the current infinite loop behavior is not really due to AutoAdvance, it's about the fact time is changing in the callback. So the following also ends up in an infinite loop:

    [Fact]
    public void AdvanceTimeInCallback()
    {
        var oneSecond = TimeSpan.FromSeconds(1);
        var timeProvider = new FakeTimeProvider();

        var timer = timeProvider.CreateTimer(_ =>
        {
            timeProvider.Advance(oneSecond);
        }, null, TimeSpan.Zero, oneSecond);

        Assert.Fail("This is never reached.");
    }

[egil]

I'll try applying a similar technique. For the record though, the current infinite loop behavior is not really due to AutoAdvance, it's about the fact time is changing in the callback.

Yeah, I understand. I was just describing the issue from a "testing production code" point of view.

Production code would not call Advance/SetUtcNow during a timer callback, but it could call GetUtcNow, and that will only advance time if auto-advance is enabled.