dotnet / dotnet/extensions

FakeTimeProvider.CreateTimer should return an implicitly rooted timer

Open
#5,765 0 comments 0 reactions 0 assignees View on GitHub
area-fundamentals untriaged
Dominant language
C#
Stars
3.2k
Forks
894
Avg merge
1d 12h
Merged PRs (30d)
23

Description

`TimeProvider.System.CreateTimer` returns a `SystemTimeProviderTimer`.

Unlike `System.Threading.Timer`, it is implicitly rooted timer, meaning that when the variable to which it was assigned is eligible for GC, it will still work until `Disposed`.

This is deliberately not the case with `FakeTimeProvider`s implementation for `CreateTimer` since it returns a version which is more like `System.Threading.Timer` and less than `SystemTimeProviderTimer`. From the comments for [`Waiter`'s class](https://github.com/dotnet/extensions/blob/20c12ef61fc33865f36c1f4f6e8e2240e8c25f32/src/Libraries/Microsoft.Extensions.TimeProvider.Testing/Waiter.cs#L8):

```
// We keep all timer state here in order to prevent Timer instances from being self-referential,
// which would block them being collected when someone forgets to call Dispose on the timer. With
// this arrangement, the Timer object will always be collectible, which will end up calling Dispose
// on this object due to the timer's finalizer.
```

But what if I forget to dispose the the Timer and depend on this implicitly-rooted behavior of timers that is promised to me for [`TimeProvider.CreateTimer`](https://learn.microsoft.com/en-us/dotnet/api/system.timeprovider.createtimer?view=net-9.0)

>
> The return [ITimer](https://learn.microsoft.com/en-us/dotnet/api/system.threading.itimer?view=net-9.0) instance will be implicitly rooted while the timer is still scheduled.
>

I think the current implementation breaks the contract from the documentation for the abstract class and will cause certain tests to fail where they won't when using the default `TimeScheduler.System`.

Short reproducible code:

```
void Main() {

TimeProvider timeProvider = TimeProvider.System;
var timerStarter = new TimerStarter();

timerStarter.Start(timeProvider, _ => Console.WriteLine("System"),
null, TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
Thread.Sleep(5000);
Console.WriteLine("--------------");
// System

var fakeTimeProvider = new FakeTimeProvider();
timerStarter.Start(fakeTimeProvider, _ => Console.WriteLine("Fake"),
null, TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
// Simulate GC during testing
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();

fakeTimeProvider.Advance(TimeSpan.FromSeconds(5));
Thread.Sleep(5000);
}

class TimerStarter {
public void Start(TimeProvider timeProvider,
TimerCallback callback,
object? state,
TimeSpan dueDate,
TimeSpan period
) {
timeProvider.CreateTimer(callback, state, dueDate, period);
}
}
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.