API to provide the current system time

[YohDeadfall]

This proposal is edited by @tarekgh

Proposal

The aim of this proposal is to introduce time abstraction. This abstraction will include the ability to retrieve the system date and time, either in UTC or local time, as well as timestamps for use in performance or tagging scenarios. Additionally, this abstraction can be used in the Task operations like WaitAsync and CancellationTokenSource CancelAfter.
By introducing this new abstraction, it will become possible to replace other existing abstractions that are currently being used in a nonuniform way across various interfaces such as ISystemClock and ITimer. The following are some examples of such interfaces:

• src/Servers/Kestrel/Transport.Quic/src/Internal/ISystemClock.cs
• SignalR/common/Shared/ISystemClock.cs
• Middleware/OutputCaching/src/ISystemClock.cs
• Security/Authentication/Core/src/ISystemClock.cs
• Middleware/ResponseCaching/src/Interfaces/ISystemClock.cs
• Servers/Kestrel/Core/src/Internal/Infrastructure/ISystemClock.cs
• libraries/Microsoft.Extensions.Caching.Abstractions/src/Internal/ISystemClock.cs
• ReverseProxy/Utilities/ITimer.cs

In addition, this new abstraction will enable the creation of tests that can mock time functionality, providing greater flexibility in testing through the ability to customize time operations.

Below are some design notes to consider:

• The proposed abstraction should be compatible with both the .NET and .NET Framework for maximum flexibility.
• It should work on down-level supported versions of both .NET and .NET Core, as it will support netstandard2.0.
• For .NET 8.0, the abstraction will be implemented in the CoreLib.
• For downl-level, the abstraction will be implemented in Microsoft.Extensions.Primitives or any chosen other OOB library which supports ns2.0 and will type forward to core if the library used for .NET 8 or up.
• In .NET 8.0, will add WaitAsync methods to the Task class which work with the time abstraction. Additionally, will provide a new CancellationTokenSource constructor that works with abstraction.
• In the down-level, we'll provide Task extension methods for WaitAsync operation and creating CancellationTokenSource support the abstraction.

APIs proposal

APIs for .NET 8.0 and Down-levels

• Note, the introduced APIs in down-levels will be type forwarded when running on .NET 8.0 and up.

namespace System
{
    /// <summary>Provides an abstraction for time. </summary>
    public abstract class TimeProvider
    {
        /// <summary>Initializes the instance. </summary>
        protected TimeProvider();

        /// <summary>
        /// Gets a <see cref="TimeProvider"/> that provides a clock based on <see cref="DateTimeOffset.UtcNow"/>,
        /// a time zone based on <see cref="TimeZoneInfo.Local"/>, a high-performance time stamp based on <see cref="Stopwatch"/>,
        /// and a timer based on <see cref="Timer"/>.
        /// </summary>
        public static TimeProvider System { get; }

        /// <summary>
        /// Creates a <see cref="TimeProvider"/> that provides a clock based on <see cref="DateTimeOffset.UtcNow"/>,
        /// a time zone based on <paramref name="timeZone"/>, a high-performance time stamp based on <see cref="Stopwatch"/>,
        /// and a timer based on <see cref="Timer"/>.
        /// </summary>
        public static TimeProvider FromLocalTimeZone(TimeZoneInfo timeZone);

        /// <summary>
        /// Gets a <see cref="DateTimeOffset"/> value whose date and time are set to the current
        /// Coordinated Universal Time (UTC) date and time and whose offset is Zero,
        /// all according to this <see cref="TimeProvider"/>'s notion of time.
        /// </summary>
        public abstract DateTimeOffset UtcNow { get; }

        /// <summary>
        /// Gets a <see cref="DateTimeOffset"/> value that is set to the current date and time according to this <see cref="TimeProvider"/>'s
        /// notion of time based on <see cref="UtcNow"/>, with the offset set to the <see cref="LocalTimeZone"/>'s offset from Coordinated Universal Time (UTC).
        /// </summary>
        public DateTimeOffset LocalNow { get; }

        /// <summary>Gets a <see cref="TimeZoneInfo"/> object that represents the local time zone according to this <see cref="TimeProvider"/>'s notion of time. </summary>
        public abstract TimeZoneInfo LocalTimeZone { get; }

        /// <summary>Gets the current high-frequency value designed to measure small time intervals with high accuracy in the timer mechanism. </summary>
        /// <returns>A long integer representing the high-frequency counter value of the underlying timer mechanism. </returns>
        public abstract long GetTimestamp();

        /// <summary>Gets the frequency of <see cref="GetTimestamp"/> of high-frequency value per second. </summary>
        public abstract long TimestampFrequency { get; }

        /// <summary>Gets the elapsed time between two timestamps retrieved using <see cref="GetTimestamp"/>. </summary>
        /// <param name="startingTimestamp">The timestamp marking the beginning of the time period. </param>
        /// <param name="endingTimestamp">The timestamp marking the end of the time period. </param>
        /// <returns>A <see cref="TimeSpan"/> for the elapsed time between the starting and ending timestamps. </returns>
        public TimeSpan GetElapsedTime(long startingTimestamp, long endingTimestamp);

        /// <summary>Creates a new <see cref="ITimer"/> instance, using <see cref="TimeSpan"/> values to measure time intervals. </summary>
        /// <param name="callback">
        /// A delegate representing a method to be executed when the timer fires.  The method specified for callback should be reentrant,
        /// as it may be invoked simultaneously on two threads if the timer fires again before or while a previous callback is still being handled.
        /// </param>
        /// <param name="state">An object to be passed to the <paramref name="callback"/>. This may be null. </param>
        /// <param name="dueTime">The amount of time to delay before <paramref name="callback"/> is invoked. Specify <see cref="Timeout.InfiniteTimeSpan"/> to prevent the timer from starting. Specify <see cref="TimeSpan.Zero"/> to start the timer immediately. </param>
        /// <param name="period">The time interval between invocations of <paramref name="callback"/>. Specify <see cref="Timeout.InfiniteTimeSpan"/> to disable periodic signaling. </param>
        /// <returns>The newly created <see cref="ITimer"/> instance. </returns>
        /// <exception cref="ArgumentNullException"><paramref name="callback"/> is null. </exception>
        /// <exception cref="ArgumentOutOfRangeException">The number of milliseconds in the value of <paramref name="dueTime"/> or <paramref name="period"/> is negative and not equal to <see cref="Timeout.Infinite"/>, or is greater than <see cref="int.MaxValue"/>. </exception>
        /// <remarks>
        /// <para>
        /// The delegate specified by the callback parameter is invoked once after <paramref name="dueTime"/> elapses, and thereafter each time the <paramref name="period"/> time interval elapses.
        /// </para>
        /// <para>
        /// If <paramref name="dueTime"/> is zero, the callback is invoked immediately. If <paramref name="dueTime"/> is -1 milliseconds, <paramref name="callback"/> is not invoked; the timer is disabled,
        /// but can be re-enabled by calling the <see cref="ITimer.Change"/> method.
        /// </para>
        /// <para>
        /// If <paramref name="period"/> is 0 or -1 milliseconds and <paramref name="dueTime"/> is positive, <paramref name="callback"/> is invoked once; the periodic behavior of the timer is disabled,
        /// but can be re-enabled using the <see cref="ITimer.Change"/> method.
        /// </para>
        /// <para>
        /// The return <see cref="ITimer"/> instance will be implicitly rooted while the timer is still scheduled.
        /// </para>
        /// <para>
        /// <see cref="CreateTimer"/> captures the <see cref="ExecutionContext"/> and stores that with the <see cref="ITimer"/> for use in invoking <paramref name="callback"/>
        /// each time it's called.  That capture can be suppressed with <see cref="ExecutionContext.SuppressFlow"/>.
        /// </para>
        /// </remarks>
        public abstract ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
    }
}

namespace System.Threading
{
    /// <summary>Represents a timer that can have its due time and period changed. </summary>
    /// <remarks>
    /// Implementations of <see cref="Change"/>, <see cref="IDisposable.Dispose"/>, and <see cref="IAsyncDisposable.DisposeAsync"/>
    /// must all be thread-safe such that the timer instance may be accessed concurrently from multiple threads.
    /// </remarks>
    public interface ITimer : IDisposable, IAsyncDisposable
    {
        /// <summary>Changes the start time and the interval between method invocations for a timer, using <see cref="TimeSpan"/> values to measure time intervals. </summary>
        /// <param name="dueTime">
        /// A <see cref="TimeSpan"/> representing the amount of time to delay before invoking the callback method specified when the <see cref="ITimer"/> was constructed.
        /// Specify <see cref="Timeout.InfiniteTimeSpan"/> to prevent the timer from restarting. Specify <see cref="TimeSpan.Zero"/> to restart the timer immediately.
        /// </param>
        /// <param name="period">
        /// The time interval between invocations of the callback method specified when the Timer was constructed.
        /// Specify <see cref="Timeout.InfiniteTimeSpan"/> to disable periodic signaling.
        /// </param>
        /// <returns><see langword="true"/> if the timer was successfully updated; otherwise, <see langword="false"/>. </returns>
        /// <exception cref="ObjectDisposedException">The timer has already been disposed. </exception>
        /// <exception cref="ArgumentOutOfRangeException">The <paramref name="dueTime"/> or <paramref name="period"/> parameter, in milliseconds, is less than -1 or greater than 4294967294. </exception>
        bool Change(TimeSpan dueTime, TimeSpan period);
    }

APIs for .NET 8.0 Only

namespace System.Threading
{
-   public sealed class Timer : MarshalByRefObject, IDisposable, IAsyncDisposable
+  public sealed class Timer : MarshalByRefObject, IDisposable, IAsyncDisposable, ITimer
    {
    }

    public class CancellationTokenSource : IDisposable
    {
+        /// <summary>Initializes a new instance of the <see cref="CancellationTokenSource"/> class that will be canceled after the specified <see cref="TimeSpan"/>. </summary>
+        /// <param name="delay">The time interval to wait before canceling this <see cref="CancellationTokenSource"/>. </param>
+        /// <param name="timeProvider">The <see cref="TimeProvider"/> with which to interpret the <paramref name="delay"/>. </param>
+        /// <exception cref="ArgumentOutOfRangeException"><paramref name="delay"/>'s <see cref="TimeSpan.TotalMilliseconds"/> is less than -1 or greater than <see cref="uint.MaxValue"/> - 1. </exception>
+        /// <exception cref="ArgumentNullException"><paramref name="timeProvider"/> is null. </exception>
+        /// <remarks>
+        /// The countdown for the delay starts during the call to the constructor.  When the delay expires,
+        /// the constructed <see cref="CancellationTokenSource"/> is canceled, if it has
+        /// not been canceled already. Subsequent calls to CancelAfter will reset the delay for the constructed
+        /// <see cref="CancellationTokenSource"/>, if it has not been canceled already.
+        /// </remarks>
+        public CancellationTokenSource(TimeSpan delay, TimeProvider timeProvider);
    }

    public sealed class PeriodicTimer : IDisposable
    {
+        /// <summary>Initializes the timer. </summary>
+        /// <param name="period">The time interval between returning the next enumerated value.</param>
+        /// <param name="timeProvider">The <see cref="TimeProvider"/> used to interpret <paramref name="period"/>. </param>
+        /// <exception cref="ArgumentOutOfRangeException"><paramref name="period"/> must be <see cref="Timeout.InfiniteTimeSpan"/> or represent a number of milliseconds equal to or larger than 1 and smaller than <see cref="uint.MaxValue"/>. </exception>
+        /// <exception cref="ArgumentNullException"><paramref name="timeProvider"/> is null</exception>
+        public PeriodicTimer(TimeSpan period, TimeProvider timeProvider);
    }
}

namespace System.Threading.Tasks
{
    public class Task<TResult> : Task
    {
+        /// <summary>Gets a <see cref="Task{TResult}"/> that will complete when this <see cref="Task{TResult}"/> completes or when the specified timeout expires. </summary>
+        /// <param name="timeout">The timeout after which the <see cref="Task"/> should be faulted with a <see cref="TimeoutException"/> if it hasn't otherwise completed. </param>
+        /// <param name="timeProvider">The <see cref="TimeProvider"/> with which to interpret <paramref name="timeout"/>. </param>
+        /// <returns>The <see cref="Task{TResult}"/> representing the asynchronous wait.  It may or may not be the same instance as the current instance. </returns>
+        public new Task<TResult> WaitAsync(TimeSpan timeout, TimeProvider timeProvider);

+        /// <summary>Gets a <see cref="Task{TResult}"/> that will complete when this <see cref="Task{TResult}"/> completes, when the specified timeout expires, or when the specified <see cref="CancellationToken"/> has cancellation requested. </summary>
+        /// <param name="timeout">The timeout after which the <see cref="Task"/> should be faulted with a <see cref="TimeoutException"/> if it hasn't otherwise completed. </param>
+        /// <param name="timeProvider">The <see cref="TimeProvider"/> with which to interpret <paramref name="timeout"/>. </param>
+        /// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for a cancellation request. </param>
+        /// <returns>The <see cref="Task{TResult}"/> representing the asynchronous wait.  It may or may not be the same instance as the current instance. </returns>
+        public new Task<TResult> WaitAsync(TimeSpan timeout, TimeProvider timeProvider, CancellationToken cancellationToken);
    }

    public class Task : IAsyncResult, IDisposable
    {
+        /// <summary>Gets a <see cref="Task"/> that will complete when this <see cref="Task"/> completes or when the specified timeout expires. </summary>
+        /// <param name="timeout">The timeout after which the <see cref="Task"/> should be faulted with a <see cref="TimeoutException"/> if it hasn't otherwise completed. </param>
+        /// <param name="timeProvider">The <see cref="TimeProvider"/> with which to interpret <paramref name="timeout"/>. </param>
+        /// <returns>The <see cref="Task"/> representing the asynchronous wait.  It may or may not be the same instance as the current instance. </returns>
+        public Task WaitAsync(TimeSpan timeout, TimeProvider timeProvider);

+        /// <summary>Gets a <see cref="Task"/> that will complete when this <see cref="Task"/> completes, when the specified timeout expires, or when the specified <see cref="CancellationToken"/> has cancellation requested. </summary>
+        /// <param name="timeout">The timeout after which the <see cref="Task"/> should be faulted with a <see cref="TimeoutException"/> if it hasn't otherwise completed. </param>
+        /// <param name="timeProvider">The <see cref="TimeProvider"/> with which to interpret <paramref name="timeout"/>. </param>
+        /// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for a cancellation request. </param>
+        /// <returns>The <see cref="Task"/> representing the asynchronous wait.  It may or may not be the same instance as the current instance. </returns>
+        public Task WaitAsync(TimeSpan timeout, TimeProvider timeProvider, CancellationToken cancellationToken);
    }
}

APIs for down-level Only

namespace System.Threading.Tasks
{
    public static class TimeProviderTaskExtensions
    {
        public static async Task<TResult> WaitAsync<TResult>(this Task<TResult> task, TimeSpan timeout, TimeProvider timeProvider);
        public static async Task<TResult> WaitAsync<TResult>(this Task<TResult> task, TimeSpan timeout, TimeProvider timeProvider, CancellationToken cancellationToken);
        public static async Task WaitAsync(this Task task, TimeSpan timeout, TimeProvider timeProvider);
        public static async Task WaitAsync(this Task task, TimeSpan timeout, TimeProvider timeProvider, CancellationToken cancellationToken);

    }

Possible APIs addition for .NET 8.0 and down-level

namespace System.Threading.Tasks
{
    public static class TimeProviderTaskExtensions
    {

        /// <summary>Initializes a new instance of the <see cref="CancellationTokenSource"/> class that will be canceled after the specified <see cref="TimeSpan"/>. </summary>
        /// <param name="timeProvider">The <see cref="TimeProvider"/> with which to interpret the <paramref name="delay"/>. </param>
        /// <param name="delay">The time interval to wait before canceling this <see cref="CancellationTokenSource"/>. </param>
        /// <exception cref="ArgumentOutOfRangeException"><paramref name="delay"/>'s <see cref="TimeSpan.TotalMilliseconds"/> is less than -1 or greater than <see cref="uint.MaxValue"/> - 1. </exception>
        /// <remarks>
        /// The countdown for the delay starts during the call to the constructor.  When the delay expires,
        /// the constructed <see cref="CancellationTokenSource"/> is canceled, if it has
        /// not been canceled already. Subsequent calls to CancelAfter will reset the delay for the constructed
        /// <see cref="CancellationTokenSource"/>, if it has not been canceled already.
        /// </remarks>
        public static CancellationTokenSource CreateCancellationTokenSource(this TimeProvider timeProvider, TimeSpan delay) ;
    }
}

End of the @tarekgh edit

The Original Proposal

Motivation

The ISystemClock interface exists in:

• Microsoft.AspNetCore.Authentication
• Microsoft.AspNetCore.ResponseCaching
• Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure
• Microsoft.Extensions.Internal

There is a small difference exists between implementations, but in most cases it can be moved out. In case of Kestrel's clock which has a specific logic inside, there could be made a new interface IScopedSystemClock which will provide the scope start time as UtcNow does now. Therefore, looks like all of them could be merged into a single class/interface and put into Microsoft.Extensions.Primitives.

The same interface often implemented by developers themselves to be used by microservices and applications utilizing dependency injection.

Having a common implementation of the data provider pattern will free users from repeating the same simple code many times and will allow to test apps in conjunction with ASP.NET internals without changing an environment.

Proposed API

The ISystemClock defines a way to get the current time in UTC timezone, and it has a simple implementation:

public interface ISystemClock
{
    DateTimeOffset UtcNow { get; }
}

public class SystemClock : ISystemClock
{
    public DateTimeOffset UtcNow => DateTimeOffset.UtcNow;
}

---

Originally proposed in dotnet/aspnetcore#16844.

[AraHaan]

Actually I do not think it is needed we already have DateTime.UtcNow. However it could be useful for those who do not know about DateTime/ DateTimeOffset.

Also what is the reasoning for DateTimeOffset.UtcNow over just DateTime.UtcNow anyway?

Edit: after further reading I understand now about the places that depend on it.

[YohDeadfall]

It's impossible to mock statics in case of tests. The last but not least is that DI means that there should be no statics and ability to replace any implementation by what you want, different and incompatible patterns to be clear.

[Joe4evr]

Probably the main reason that interface exists internally in 4 separate Microsoft libraries is because MS doesn't want their building blocks to take dependencies on third-party packages, if they can at all avoid it. For any project outside of MS, it's more recommended to use NodaTime if you need to do any serious date/time math or just want semantically better date/time types to work with (including IClock that you're asking for).

[davidfowl]

It's super important for testing. Mocking things that are non deterministic is important for testing things like timeouts.

[GrabYourPitchforks]

I'd probably suggest a SystemClock.Instance singleton instead of forcing everybody to new up an instance of it. But the idea is sound on its face. Any objections to moving this forward to "ready for review"?

Edit: The singleton would look like:

public static sealed SystemClock : ISystemClock
{
    private SystemClock();
    public static ISystemClock Instance { get; }
    public DateTimeOffset UtcNow { get; }
}

In theory this would also allow the JIT to devirtualize SystemClock.Instance.UtcNow. Though not quite sure why you'd want to do that over DateTimeOffset.UtcNow. :)

[ericstj]

@davidfowl what's your recommendation here, should we expose this in Microsoft.Extensions namespace in Microsoft.Extnsions.Primitives

[davidfowl]

System.* namespace it feels that core

[danmoseley]

We have plenty of API it would be good to mock, but is either sealed or static. Example: File. It seems it wasn't a primary design criterion way back. Should we be thinking holistically about our attitude towards mockability, rather than thinking about just this one type? It might not need to be solved through API for example this proposal. Even if we want to enable it through API design, we should have a consistent pattern so it's discoverable, useable etc. I haven't kept up with the latest in mocking/DI - do we have established patterns?

cc @bartonjs do we have design guidelines? cc @stephentoub

[bartonjs]

We either don't have guidelines here, or our soft stance is "mockability isn't important", whichever one feels better to you 😄.

The guidelines we do have would say that there shouldn't be an ISystemClock interface, because it's representing a type more than a capability. (DO favor defining classes over interfaces. ... then some exceptions), and end up with

public class SystemClock
{
    public virtual DateTimeOffset UtcNow => DateTimeOffset.UtcNow;
}

If we wanted to aggressively support mockability, we'd leave it at that, but then someone would ask (perhaps not for this feature, since there's already DateTimeOffset.UtcNow, but let's keep playing the scenario out) why everyone has to create their own version of SystemClock to ask what the time is, because that's very GC inefficient. So we'd add a static Instance property:

public class SystemClock
{
    public static SystemClock Instance { get; } = new SystemClock();
    public virtual DateTimeOffset UtcNow => DateTimeOffset.UtcNow;
}

And now we're back at being non-mockable, because "everything" is going to just call SystemClock.Instance.UtcNow to get the time.

As a suggestion to bring back a semblance of mockability, someone might propose making the singleton property settable, or having some other set mechanism. Depending on who is present at the meeting, it'll either be a drawn out conversation about the value of such an API, or immediately shot down as us having learned our lessons about mutable statics.

Also, we may decide to slap a "Provider" (or "Factory" or similar) suffix on the type name, since it's actually a system time factory, not a system time.

It's a massive oversimplification, but the BCL is designed with declarative programming models, and mockability requires looser coupling with "magic"/ambient state, like a DI system.

So, in the end, I'd say "perhaps this belongs alongside a DI technology", though that then makes a tight coupling between a consumer of the time and the DI layer. Then I'd say "maybe one level higher than that?" which is, perhaps, Microsoft.Extensions or Microsoft.Extensions.Primitives. (I'd then go on to suggest that mocking time seems weird, and that (as a declarative programmer) it seems like whatever things are pulling from DateTimeOffset.UtcNow just want to take the time as an input... except for things like logging, where you really want "now" (whatever that means) vs "mocked now"... how to do that in a DI system is, as we say in mathematics, an exercise left to the reader (which then leads to this proposal, and we go 'round and 'round)).

I'm pretty sure I've said this before for a laugh, and I'll do so again: "Should I just say 'no' now and save us all 15 minutes? 😄"

[danmoseley]

I confirmed offline with @davidfowl that he's in agreement with my thoughts above that attempting to add mockability one type at a time is unlikely to give a good result. This doesn't help any place where we ourselves use the API directly. If we do anything here it should start with thinking about mockability for the core libraries as a whole and coming up with guidelines or technology that doesn't lead to a proliferation of types and overloads and that we're confident would be a significant value. Meanwhile if we're looking for a type that the core libraries themselves don't depend on, any mocking library could provide it.

[FiniteReality]

While a single, re-usable definition of a "clock" would be useful, I feel the only main uses would be unit testing, which I suppose could be done using compile-time definitions, e.g.:

using System;
using Another.Thing;

#if MOCK_TIME
using SystemClock = My.Custom.DateTimeOffset;
#else
using SystemClock = System.DateTimeOffset;
#endif

// etc. using SystemClock.UtcNow

The only disadvantage to this would be that you're not strictly testing the same code you're about to publish

[Clockwork-Muse]

@FiniteReality - If I was worried about a dependency on getting the current time, but was bound to only first-party libraries (that is, no NodaTime), I'd instead inject a Func<System.DateTimeOffset> for everything that was going to get the current time. Saves looking for weird redefinitions, and makes it really easy to deal with injection/forwarding.

Note that, while mocking the current system time is useful for testing, perhaps the bigger area is for making sure things are consistent with regards to a timezone (or here, offset). That is, rather than setting the current timezone in a context object so you can use System.DateTimeOffset.Now, you pass in something to get it for you.

[danmoseley]

@GrabYourPitchforks thoughts about my note above? Wondering whether this ought to be un-marked api-ready-for-review as several folks aren't sure this is a good direction for the base libraries. (Unless you want to use the tag to trigger a larger discussion in that group about mocking)

[GrabYourPitchforks]

@danmosemsft As manager, isn't it your privilege to say "Let's spin up a working group!"? :)

I'd be supportive of this if we wanted to have a wider discussion regarding mockability within the BCL. Though honestly the Microsoft.Extensions.* libraries have become something of a dumping ground for these sorts of APIs, so I don't know how much discussion need take place. We still consider these libraries separate from / at arm's length from the BCL.

[terrajobst]

We could expose a new type, but what does this mean for the existing packages that already defined their own version of ISystemClock, like auth? Would we break them?

[Aaronontheweb]

Inside the Akka.NET project we have had our own virtual time abstraction (TestScheduler) for solving these same types of issues during testing - I am thrilled at the prospect of being able to delete that because frankly it's a half-measure: it only works for our internal scheduler, but not other time-driven events we interact with (i.e. external Tasks.)

The benefits I'd see us realizing from this API, were it to be introduced as currently proposed:

1. Eliminating our own TestScheduler abstraction and instead, having a standard way of virtualizing time throughout our test suite;
2. Overall improved execution time for our test suite, as we can eliminate numerous Task.Delay calls we use for things like simulating time-driven stages in Akka.Streams - this will help tighten the feedback loop for our developers and also significantly reduce the amount of CI/CD time we consume every day;
3. Elimination of race conditions in those same tests, which are frustrating / annoying / often irresolvable without the ability to virtualize the .NET scheduler.

I can't tell you how much time I've personally wasted trying to fix these racy tests due to scheduling imprecision in our suite - hundreds of hours over the past ten years probably. Framework authors will appreciate this feature - others probably won't interact with it much. Please add it!

[stephentoub]

@ploeh, thank you for the feedback and for your follow-up response. I appreciate it.

Ultimately, however, I expect (and hope) that I can simply ignore this new API and suffer no adverse effects.

I would hope in time (no pun intended) you'd find use for it. But you can certainly ignore it if it's not something you need, and I don't foresee any adverse effects to doing so; nothing forces you to use it, e.g. no APIs in the core libraries will ever require you to supply one. It's entirely optional.

[stephentoub]

@Aaronontheweb and @egil, it's great to hear this should suffice for your needs. Just to confirm, is there anything critical missing in the abstraction for your needs? Once it ships, it would be great if it could entirely replace the heart of those custom abstractions.

[stephentoub]

After using TimeProvider a bit, I think we should consider a few tweaks (most of these were things we discussed as possibilities and said we’d revisit after experimentation with the initial accepted design, and some of which is reflected in feedback in this thread):

• Base implementation. We make all the abstract members instead be virtual. We’d then make their base implementations be the system implementations. This makes it easier to customize the default behavior, only needing to override the things you care about (it shouldn’t impact mocking at all one way or the other). We can still keep the type abstract, in order to make it clear that there’s zero benefit to instantiating it directly, but we could remove the abstractness if there’s a good reason to. If we do make it non-abstract, the System singleton would just be an instance of the base type. If we leave it abstract, the internal SystemTimeProvider simply becomes private sealed class SystemTimeProvider : TimeProvider { }, as all of its implementation is the base implementation.
• Delete FromLocalTimeZone. This is such a niche use case, it’s not worth cluttering up the surface area of the type for or breeding choice when someone does TimeProvider. in an IDE, e.g. I don’t want someone accidentally thinking they should be doing TimeProvider.FromLocalTimeZone(TimeZoneInfo.Local) or something like that. For someone who does have this scenario, once the system implementation is the base, they can achieve it with their own simple derived type:

sealed class LocalTimeProvider(TimeZoneInfo local) : TimeProvider
{
    public override LocalTimeZone => local;
}

• Parameterless ctor. Delete the existing (int) constructor and just make it parameterless, and either a) make TimestampFrequency virtual or b) add a protected virtual TimestampFrequencyCore (where TimestampFrequency would then invoke TimestampFrequencyCore once and store the result). We ended up with the ctor design so as to help ensure that frequency is guaranteed to be a constant, since we don’t envision or want to support a system where frequency changes (it would make it impossible to understand timestamps if that’s possible). We can achieve the same thing with FrequencyCore, or we can just state that an override should return the same value every time and leave it at that. And with the base implementation being the system implementation, the need for someone to override this should be practically never other than if they want to test their system’s correct handling of manual use of frequency values. As we cited as a possible issue when we were discussing the ctor, having no parameterless ctor does make mocking a tad bit more complicated, and as that's a primary scenario, removing any road blocks there is a good thing.
• Methods. I could go either way on this one, whether keeping UtcNow and LocalNow properties or making them GetUtcNow() and GetLocalNow(). I lean towards methods, for consistency within the class itself with GetTimeStamp(), because if we were designing everything from scratch they would be methods, because we do expect their values to change automatically between some invocations, etc.

What’s currently checked in is:

namespace System
{
    public abstract class TimeProvider
    {
        public static TimeProvider System { get; }
        public static TimeProvider FromLocalTimeZone(TimeZoneInfo timeZone);

        protected TimeProvider(long timestampFrequency);

        public abstract DateTimeOffset UtcNow { get; }
        public DateTimeOffset LocalNow { get; }
        public abstract TimeZoneInfo LocalTimeZone { get; }
        public long TimestampFrequency { get; }
        public abstract long GetTimestamp();
        public TimeSpan GetElapsedTime(long startingTimestamp, long endingTimestamp);
        public abstract ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
    }
}

If we adopt all that, it would instead be massaged to be:

namespace System
{
    public abstract class TimeProvider
    {
        public static TimeProvider System { get; }

        protected TimeProvider();

        public virtual DateTimeOffset GetUtcNow();
        public virtual long GetTimestamp();
        public virtual long TimestampFrequency { get; } // or `public long TimestampFrequency { get; }` and `protected virtual long TimestampFrequencyCore { get; }`
        public virtual ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
        public virtual TimeZoneInfo LocalTimeZone { get; }

        public DateTimeOffset GetLocalNow();
        public TimeSpan GetElapsedTime(long startingTimestamp, long endingTimestamp);
    }
}

[Aaronontheweb]

@Aaronontheweb and @egil, it's great to hear this should suffice for your needs. Just to confirm, is there anything critical missing in the abstraction for your needs? Once it ships, it would be great if it could entirely replace the heart of those custom abstractions.

Best way for me to express that is with an example - this is one of the aforementioned racy specs in our suite:

public void KeepAlive_must_emit_elements_periodically_after_silent_periods()
{
    this.AssertAllStagesStopped(() =>
    {
        var sourceWithIdleGap = Source.Combine(Source.From(Enumerable.Range(1, 5)),
            Source.From(Enumerable.Range(6, 5)).InitialDelay(TimeSpan.FromSeconds(2)),
            i => new Merge<int, int>(i));

        var result = sourceWithIdleGap
            .KeepAlive(TimeSpan.FromSeconds(0.6), () => 0)
            .Grouped(1000)
            .RunWith(Sink.First<IEnumerable<int>>(), Materializer);

        result.Wait(TimeSpan.FromSeconds(3)).Should().BeTrue();
        result.Result.Should().BeEquivalentTo(
            Enumerable.Range(1, 5).Concat(new[] {0, 0, 0}).Concat(Enumerable.Range(6, 5)));
    }, Materializer);

The code being tested behind the scenes relies on our scheduler operating on a fixed interval - checking for idleness in the stream and injecting "keep-alive" values when pauses are detected.

With these new methods, would I be able to create my own TimeProvider abstraction that can do the following:

1. Artificially advance the current DateTime.UtcNow values in order to simulate the pauses we're looking for;
2. (optional, but really helpful) Trigger the appropriate number of "ticks" using a built-in .NET BCL timer when we artificially advance the clock, such as the PeriodicTimer? We're looking at replacing our built-in scheduler implementation with the PeriodicTimer - this would be a tremendous value add for testability purposes if that were the case.

Thanks so much for your time @stephentoub

[stephentoub]

would I be able to create my own TimeProvider abstraction that can do the following

Yes... but you shouldn't even need to create your own for that. Our intention is to ship a ManualTimeProvider (or FakeTimeProvider or choose some other suitable name) that lets you set/change/advance/etc. the current time, which will of course not only update what's returned from Utc/LocalNow but also invoke all the relevant timers that need to be invoked and the appropriate number of times (based on dueTime and period). I expect it's very similar to what your implementation already does.

[Aaronontheweb]

Our intention is to ship a ManualTimeProvider (or FakeTimeProvider or choose some other suitable name) that lets you set/change/advance/etc. the current time, which will of course not only update what's returned from Utc/LocalNow but also invoke all the relevant timers that need to be invoked and the appropriate number of times (based on dueTime and period)

Music to my ears. Sounds perfect.

[mattjohnsonpint]

FWIW, I'll probably end up publishing a nuget of a properly implemented NetworkTimeProvider layered on top of this, similar to my existing NodaTime.NetworkClock package. I wouldn't expect this to be baked-in, but just wanted to point out that the system implementation might not necessarily be the only one used in production.

The LocalTimeProvider mentioned previously would be another example, for apps that always run with the context of a single time zone other than TimeZoneInfo.Local.

[slang25]

The suggested improvements are excellent, when I saw Nick Chapsas recent video, the constructor was his standout criticism and I think it was a fair one.

In general there still seems to be a theme (as @CZEMacLeod has pointed out), the TimeProvider type enables some really valuable functionality that was otherwise really difficult (the testing Cancellation timers was a perfect example), but for a lot of scenarios, the usual IClock 2 method abstraction is all that is wanted.

@stephentoub - is this something that is recognised? Would having an lighter interface and letting people chose the level of complexity they need not be a viable option? In an earlier comment you said you didn't want to subdivide the API method by method and this is a single abstraction to flow the concept of time, but it seems to me like there is a high-level and low-level split.

[tarekgh]

@slang25 with the updated proposal in the comment #36617 (comment), could you explain what complexity you will have when using TimeProivder compared to using IClock?

[eatdrinksleepcode]

I have seen this come up a couple of times now:

Usually for tests you're going to be using Moq (or some other mocking library), and you only need to mock the methods you're going to actually call.

even for testing, we expect the majority of use will be either via mocking (where you only need to supply implementations for the things you care about)...

Partially implementing/mocking dependencies is an anti-pattern that leads to fragile tests. Please do not promote this, or design around it as a testing practice.

A simple example in this case would be code that initially uses GetUtcNow(), and so all of the tests that execute that production code in a controlled manner (which may include unit tests but also isolated end-to-end tests for example) provide a mock with that one member implemented, and nothing else. Later on someone realizes that they were only using UtcNow to track the passage of time between two points of code, and GetTimestamp() is a better way to do that. So they make a small to change the production code, expecting that nothing will break because the actual behavior of the code hasn't changed - but all of the tests break, because the mock doesn't provide the new member. What should have been a simple change suddenly has a huge blast radius, and the dev doesn't feel like spending a lot of time right now on what they thought was a quick change; so they leave it and move on, and the code continues to ossify.

This is not a hypothetical fear; other than too many high-level tests which are fragile and take too long to run, this is the pathological failure case of automated testing that I have observed as it has finally become standard practice in the last decade+. Instead of being the enabler for safe, rapid change that it is supposed to be, the test suite becomes one of the primary roadblocks to change.

This is also the practical impact of the SRP conversations that have been discussed abstractly in this thread. It's not just about "are all of these things related to the same concept" or "does the implementation of these things need to change together"; it's also about "are these things generally used together, and if not what impact does that have on the consumer who has to care about things they don't need". It has been asserted that consumers can simply ignore what they don't need; but it's often not that simple. The more pieces a dependency has, the harder it is to implement, which means the harder it is to fake; and the more likely a consumer is to bail out to partial mocking as a shortcut (especially if they have not yet been burned by it and don't know to avoid id), setting themselves up for pain in the future.

Having a standard FakeTimeProvider can definitely help mitigate this problem, if there is a single mechanism for controlling the time of the fake that affects all of the different use cases. If the way you manually control GetUtcNow() is separate from the way you control GetTimestamp() (via individual Set* methods for example), then we're right back at the problem I described earlier. It also has to be promoted as the preferred mechanism. Given that the primary purpose of this proposal is to enable testability, I think the API and usage examples of that FakeTimeProvider should be part of the proposal; even if it doesn't actually ship, how it is used is the primary measure of how effective the proposal is.

Overall, I am happy the problem of testing time is being addressed, and TimeProvider as currently proposed is definitely a lot better than nothing. But I think combining use cases which are generally very different from a consumer's perspective (even if they are similar from an implementation perspective) is not ideal, and there are opportunities to better tailor the API surface to those use cases.

[slang25]

@slang25 with the updated proposal in the comment #36617 (comment), could you explain what complexity you will have when using TimeProivder compared to using IClock?

It's not the usage of it that's the issue, some will say "I dot into it and see a bunch of things I'm not looking for", well yes you can just ignore it. The problem is the concept count and perceived complexity, let's say I'm defining some domain logic that checks if something has expired, I wan't my domain to be as clear as possible. Taking an IClock gives this clarity, later in the same application I have some client code to interact with a data store and I want to test timeouts etc... then I want the low-level API (TimeProvider).

Saying one size fits all would work, for sure, but I think this is where the community frustration is stemming from, there are different contexts requiring different levels of abstraction. A high-level and a low-level.

If this is fundamentally problematic then fine, but it seems to me that it solves a lot of the community feedback so far.

[niemyjski]

I just wanted to share a SystemClock and tests that we've used and battle tested in case it helps.

https://github.com/FoundatioFx/Foundatio/blob/master/src/Foundatio/Utility/SystemClock.cs
https://github.com/FoundatioFx/Foundatio/blob/master/tests/Foundatio.Tests/Utility/SystemClockTests.cs

[egil]

@Aaronontheweb and @egil, it's great to hear this should suffice for your needs. Just to confirm, is there anything critical missing in the abstraction for your needs? Once it ships, it would be great if it could entirely replace the heart of those custom abstractions.

My use cases thus far are covered.

I do think it would be better to have two protected constructors for TimeProvider, one that takes a timestamp frequency and a default one that just sets whatever is the default for the platform.

[jskeet]

I'm astonished that I've only just become aware of this issue - I'm really pleased to see it being discussed.

Consistent abstractions

One point from earlier, by @aredfox:

I would like to point out though that it would be helpful for projects like NodaTime, and others, and interface such as ITimeProvider would be a lot easier to implement their own provider, and thereby implement the correct translations and features accordingly.

I'm not sure I agree with that. I'd absolutely expect any interface/abstract class for this feature to express itself in BCL types - DateTime, DateTimeOffset etc - whereas Noda Time's IClock interface exposes an Instant. There's code to convert between the two sets of abstractions, of course, but I'd be reluctant to make Noda Time implement an interface that is described in the BCL abstractions.

What I'm more likely to do is create a parallel interface/abstract class to whatever the BCL provides, which exposes the same functionality in Noda Time concepts, along with adapters between the two... assuming that's possible. I'd really like Noda Time users to be able to write code that:

• Is thoroughly testable without long pauses
• Uses Task.Delay or equivalent functionality
• Doesn't require them to change abstraction (between BCL and Noda Time types) more than is absolutely necessary

Currently users who only need "current instant" functionality can do that with IClock, but I'd love "passage of time" functionality too (with delays expressed in Duration rather than TimeSpan).

Of course this assumes that Noda Time will still be required in the future. One little idea is that if .NET is going to get all of this new functionality, maybe it would be a good time to introduce a System.Time namespace with new abstractions that don't have the issues of DateTime etc. That's only a one-tenth serious suggestion - I'm really not expecting it to happen, but I thought I'd mention it. It would be a real shame to ship this new shiny wonderful testability with an old abstraction and then decide to create a new abstraction for .NET 9 :)

Clocks and zones

One other minor correction to a comment from @Clockwork-Muse:

NodaTime/JSR310 include the current system or process timezone as an additional property on the clock. Because otherwise you require another injection point for "current offset/rules". We should probably do the same.

JSR-310 does (Clock.getZone() but Noda Time doesn't. Our IClock interface is deliberately machine-time-based, with just a single Instant GetCurrentInstant() method. We also provide a ZonedClock which wraps an IClock, DateTimeZone and CalendarSystem together, which is just a convenience to allow things like "What's today's date?" with a testable clock. We don't currently have anything that makes "what's the system time zone" itself more testable - I'd normally expect that to be called once during dependency injection, just like using SystemClock.Instance. That may be an oversight on my part.

[Clockwork-Muse]

My bad.

I'd normally expect that to be called once during dependency injection, just like using SystemClock.Instance

... and now I'm rethinking one of my earlier comments around the System provider, because the anticipated default here is going to allow it to automatically handle when the user changes their timezone... rare though that usually is.

[rodion-m]

maybe it would be a good time to introduce a System.Time namespace with new abstractions that don't have the issues of DateTime etc.

Forgive my naivete, but what is stopping us from just adding NodaTime's abstractions and classes to the new System.Time namespace? If @jskeet doesn't mind, of course :)

[jskeet]

Forgive my naivete, but what is stopping us from just adding NodaTime's abstractions and classes to the new System.Time namespace? If @jskeet doesn't mind, of course :)

Due diligence, for starters :) I would have absolutely no problem with .NET making Noda Time effectively obsolete... and hopefully doing a better job of it in some cases! (There are i18n aspects where I simply don't know enough to do the job fully.) But just like JSR-310 started with Joda Time, looked at it carefully and then came out with something clearly related but different, I'd expect the same to happen with a System.Time set of abstractions.

[Clockwork-Muse]

Part of the problem is that that got debated and mostly shut down years ago: #14744 . We ended up with new date/time types, but no new namespace, and no real motion towards one either.....