search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

API to provide the current system time #36617

Closed YohDeadfall closed 1 year ago

YohDeadfall commented 4 years ago

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:

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:

APIs proposal

APIs for .NET 8.0 and Down-levels

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:

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

GrabYourPitchforks commented 4 years ago

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 commented 4 years ago

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

davidfowl commented 4 years ago

System.* namespace it feels that core

danmoseley commented 4 years ago

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 commented 4 years ago

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

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? :smile:"

danmoseley commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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?

FiniteReality commented 4 years ago

Since ISystemClock is usually an internal type (which is forced to be public, IIRC) I don't think people would be too upset if there was a breaking change for 5.0 or even 6.0, since that's a major version bump.

ChadNedzlek commented 4 years ago

If anyone does any sort of testing, they almost certainly used these types, even if they are internal; I know I did in several important projects. Though I'd accept the breaking change if it meant this thing would never break again, because it was really supported instead of tacked on to several different libraries.

I'd have to change code in a few different projects to deal with this, and it would be an annoying change that only breaks at runtime (like all DI changes), which is a really frustrating class of changes to have to deal with, because often we fail to notice before rolling the change out to production.

Clockwork-Muse commented 4 years ago

@bartonjs - I didn't comment on this earlier:

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.

... normally the way something like this would be used is as a singleton per application (Or, in a web context, cached singleton per resolved user timezone), and injected into all methods requiring "getting" current time (note that most methods that "use" current time should be instead accepting it as a parameter). Am I missing something about why this would be GC unfriendly? Or were you expecting all the relevant methods to be new-ing up their own copy, or something?

terrajobst commented 4 years ago

Video

@maryamariyan @davidfowl would we be OK with a breaking change to reconcile the ISystemClock types into a single core type? If not, we shouldn't do this. If yes, then I think we can make progress.

YohDeadfall commented 4 years ago

@terrajobst, please fix the link. The right one is https://youtu.be/WW65sGkDINQ?t=2548 (starts at 4:28).

davidfowl commented 4 years ago

@terrajobst Yes I think we need to pick when to do it but we should have a core type that does this. I've now seen lots of examples of this and think it's required (especially for testing). There are likely other things to look at besides the UtcNow (like timers and TickCount etc)

dzmitry-lahoda commented 4 years ago

Mention ISC in Reactive. Also I was on 5 projects, and each of these created their own... image

https://github.com/search?l=C%23&q=org%3Adotnet+%22interface+ISystemClock%22&type=Code

UPDATE: Milestone 6 to add interface? 5 not released yet, not all will grab it fast. So 2+ years to simple shared interface.......

davidfowl commented 4 years ago

There are a few more time based APIs that are worth considering here that we hit in YARP as well:

When writing unit tests that try to use these APIs it's important to be able to provide fake implementations (our own time machine) rather than having to delay tests waiting for time to pass.

cc @Tratcher @alnikola

alnikola commented 4 years ago

The main delays, we are observing currently, are caused by real Timers used in EntityActionSchedulerTests. I filed an issue microsoft/reverse-proxy#508 to replace it with an abstract ITimer interface.

alnikola commented 4 years ago

To clarify the above. Having a clock abstraction built directly into BCL would be really convenient since testing a time dependent code is a common problem in many of systems. On top of that, it would also be very handy to have a ITimerFactory and ITimer abstractions, too.

mattjohnsonpint commented 3 years ago

Hi. I've invested some effort exploring this in detail and would like to share my opinions and results.

First, I want to say that I understand that normally one doesn't want to spend time implementing a feature before it's gone through the full API design review process. However, in this case it seemed like much of the discussion has occurred already and the the best way to move forward was to actually implement it and see how it panned out. I have working and tested code that implements the revised proposal below. You can review it in #48681. I do not expect it to be merged at this time.

Let's go point by point:

The proposed API is below, followed by some usage examples.

Revised API Proposal

// This is the low-level abstraction for objects that provide a current UTC-based time value.
// It is an abstract class rather than an interface, as per design guidelines.
// The two provided implementations are ActualSystemClock and VirtualClock (described below).
// Additional implementations (such as an NtpClock) could be added later or created by others.
public abstract class TimeClock
{ 
    // There are two public methods that callers can use to get the current time - one that
    // returns a DateTime, and one that returns a DateTimeOffset.  Both are UTC based.
    // They are methods instead of properties, as some implementations (like VirtualClock)
    // can mutate internal state of the clock.
    // Having two different methods prevents errors such as calling .DateTime instead of
    // .UtcDateTime on a DateTimeOffset result (the difference being Unspecified vs Utc DateTimeKind).
    public DateTime GetCurrentUtcDateTime();
    public DateTimeOffset GetCurrentUtcDateTimeOffset();

    // Implementations are only required to implement the following method.
    protected abstract DateTimeOffset GetCurrentUtcDateTimeOffsetImpl();

    // However, there's also an internal fast path that is implemented by the ActualSystemClock
    // This is invoked by DateTime.UtcNow via the current TimeContext (more on this later).
    internal virtual DateTime GetCurrentUtcDateTimeImpl();
}

// This implementation always returns the actual system time from the underlying OS.
// (The internal OS-specific native functions can move from DateTime to this class.)
// The name was chosen to prevent conflicts with exiting "SystemClock" classes, and to provide a clue
// to its behavior - it is always the clock that is actually provided by the operating system.
public sealed class ActualSystemClock : TimeClock
{
    // no public construction
    private ActualSystemClock();

    // a singleton instance is used in all cases
    public static ActualSystemClock Instance { get; }
}

// This implementation is used to provide artificial time values, and is primarily intended for testing.
// The name "Virtual Clock" has often been used to describe this design pattern, and thus makes a great
// choice for the class name (vs "FakeClock", "TestClock", etc.)
public sealed class VirtualClock : TimeClock
{
    // A variety of constructors can be used.  These allow creation of a clock that returns either
    // a fixed value always, or one that ticks forward by an amount or a function with each usage.
    public VirtualClock(DateTimeOffset timeValue);
    public VirtualClock(DateTimeOffset initialTimeValue, TimeSpan advancementAmount);
    public VirtualClock(DateTimeOffset initialTimeValue, Func<DateTimeOffset, DateTimeOffset> advancementFunction);

    // There are also DateTime-based variations of the constructors, which prevents usage errors from implicit
    // conversion from DateTime to DateTimeOffset, especially when DateTime.Kind is Local or Unspecified and
    // the current TimeContext has changed the local system time zone (more on this below).
    public VirtualClock(DateTime timeValue);
    public VirtualClock(DateTime initialTimeValue, TimeSpan advancementAmount);
    public VirtualClock(DateTime initialTimeValue, Func<DateTimeOffset, DateTimeOffset> advancementFunction);
}

// This class provides an ambient context (implemented with AsyncLocal) that can be used to run existing
// code using a specific TimeClock, or a specific TimeZoneInfo (affecting which time zone is the "local" time
// zone), or both. When code is run under a TimeContext, all existing properties and methods that work with the
// current time and/or the local time zone are routed through this class to the appropriate clock or time zone.
// For example, DateTime.UtcNow might be redirected to GetCurrentUtcDateTime of a VirtualClock, which could
// be used to validate application behavior on leap days or other important dates.
// Likewise, DateTime.ToLocalTime might use a new time zone instead of the actual system local time zone.
// The local time zone functionality helps for testing, such as application behavior during DST transitions,
// and also allows for other useful non-testing scenarios,such as setting the local time zone for the lifetime
// of an HTTP request.
public sealed class TimeContext
{
    // no public construction
    private TimeContext();

    // Gets the current ambient time context (there will always be one).
    // The default context uses the actual system clock and actual system local time zone.
    public static TimeContext Current { get; }

    // Some properties to return the actual clock and local time zone.
    // The former is just for convienience/consistency (pointing to ActualSystemClock.Instance),
    // but the latter is required here since TimeZoneInfo.Local might be redirected to a differnet time zone.
    public static ActualSystemClock ActualSystemClock { get; }
    public static TimeZoneInfo ActualSystemLocalTimeZone { get; }

    // Some convienience properties to check if the actual clock and local time zone are active.
    // This is just slightly easier than comparing the values from the current context.
    public static bool ActualSystemClockIsActive { get; }
    public static bool ActualSystemLocalTimeZoneIsActive { get; }

    // Gets the clock and local time zone in use by the time context.
    // Usage is like this: TimeContext.Current.Clock or TimeContext.Current.LocalTimeZone.
    public TimeClock Clock { get; }
    public TimeZoneInfo LocalTimeZone { get; }

    // The below Run / RunAsync methods actually kick off a new time context and apply it
    // to the given action or function.  The user doesn't create the TimeContext directly,
    // but rather supplies either a clock, a time zone, or both, along with an action or
    // function to run.  I've pivitod on sync/async and return-value/no-return-value.
    // (These could be expanded further given CancelationToken or other parameters to pass through.)

    public static void Run(TimeClock clock, Action action);
    public static void Run(TimeZoneInfo localTimeZone, Action action);
    public static void Run(TimeClock clock, TimeZoneInfo localTimeZone, Action action);

    public static TResult Run<TResult>(TimeClock clock, Func<TResult> function);
    public static TResult Run<TResult>(TimeZoneInfo localTimeZone, Func<TResult> function);
    public static TResult Run<TResult>(TimeClock clock, TimeZoneInfo localTimeZone, Func<TResult> function);

    public static Task RunAsync(TimeClock clock, Func<Task> function);
    public static Task RunAsync(TimeZoneInfo localTimeZone, Func<Task> function);
    public static Task RunAsync(TimeClock clock, TimeZoneInfo localTimeZone, Func<Task> function);

    public static Task<TResult> RunAsync<TResult>(TimeClock clock, Func<Task<TResult>> function);
    public static Task<TResult> RunAsync<TResult>(TimeZoneInfo localTimeZone, Func<Task<TResult>> function);
    public static Task<TResult> RunAsync<TResult>(TimeClock clock, TimeZoneInfo localTimeZone, Func<Task<TResult>> function);
}

Some Example Usages

Using a fixed-value VirtualClock. Leap day testing shown here:

TimeClock clock = new VirtualClock(new DateTime(2024, 2, 29));
TimeContext.Run(clock, () => {
    var dt = DateTime.Now;      // 2024-02-29T00:00:00
});

Using a incrementing VirtualClock. Y2K testing shown here:

var initialValue = new DateTimeOffset(1999, 12, 31, 23, 59, 59, TimeSpan.Zero);
var advancementAmount = TimeSpan.FromSeconds(1);
TimeClock clock = new VirtualClock(initialValue, advancementAmount);
TimeContext.Run(clock, () => {
    var dt1 = DateTime.UtcNow;  // 1999-12-31T23:59:59Z
    var dt2 = DateTime.UtcNow;  // 2000-01-01T01:00:00Z
});

Using a incrementing VirtualClock with a specific time zone. End-of-DST testing shown here:

var initialValue = new DateTime(2020, 11, 1);
var advancementAmount = TimeSpan.FromHours(1);
TimeClock clock = new VirtualClock(initialValue, advancementAmount);
TimeZoneInfo tz = TimeZoneInfo.FindSystemTimeZoneById("Central Standard Time"); // "America/Chicago" on Linux
TimeContext.Run(clock, tz, () => {
    var dto1 = DateTimeOffset.Now;  // 2020-11-01T00:00:00-05:00 (CDT)
    var dto2 = DateTimeOffset.Now;  // 2020-11-01T01:00:00-05:00 (CDT)
    var dto3 = DateTimeOffset.Now;  // 2020-11-01T01:00:00-06:00 (CST)
    var dto4 = DateTimeOffset.Now;  // 2020-11-01T02:00:00-06:00 (CST)
});

Dependency injection approach.

Service implementation:

public class MyService : IMyService
{
    private readonly TimeClock _clock;

    public MyService(TimeClock clock)
    {
        _clock = clock;
    }

    public int SomeFunction()
    {
        DateTimeOffset utcNow = _clock.GetCurrentUtcDateTimeOffset();
        // ...
    }
}

DI registration:

services.AddSingleton<TimeClock, ActualSystemClock>();
services.AddSingleton<IMyService, MyService>(); //or AddScoped, etc.

Unit Tests:

[Fact]
public void SomeTest()
{
    var clock = new VirtualClock(new DateTime(2024, 2, 29)); 
    var myService = new MyService(clock);
    var result = myService.SomeFunction();
    Assert.Equal(expected, result);
}

We can also combine these approaches if we need to apply a different local time zone when testing a service that injects a TimeClock. Async works too.

[Fact]
public async Task SomeAsyncTest()
{
    await TimeContext.RunAsync(someTimeZone, async () =>
    {
        var clock = new VirtualClock(new DateTime(2020, 11, 1)); 
        var myService = new MyService(clock);
        var result = await myService.SomeAsyncFunction();
        Assert.Equal(expected, result);
    });
}

Other Approaches

I explored but rejected the following other approaches:

Ultimately, a lot of these arguments are also about Single Responsibility Principle, thus why I landed on TimeContext being a separate class, even if somewhat related.

I look forward to feedback. Thanks.

mattjohnsonpint commented 3 years ago

@tarekgh

YohDeadfall commented 3 years ago

It's a really well thought solution for the problem, thank you for working on it!

The only thing I strongly disagree with is Run methods which lead to additional allocations and flexibility loss since only in case of a static closure no allocations will happen, but if it captures some variables which will happen most of the time there will be at least two allocations: for a delegate and a closure instance.

With asynchronous Run methods things are getting worse because there's ValueTask conversion from which will lead to creation of Task while no allocations were involved during method execution.

Therefore, instead of Run methods I propose to add a few methods to create a context without public constructor exposure:

public static class TimeContext
{
    public static Disposable WithClock(TimeClock clock);
    public static Disposable WithClockAndTimeZone(TimeClock clock, TimeZoneInfo timeZone);
    public static Disposable WithTimeZone(TimeZoneInfo timeZone);

    // It's a struct for zero allocations and storing previous values of clock and zone
    // in place: on the stack for sync methods and in the same region of memory
   // for asynchronous methods involving a boxed state machine.
    public readonly struct Disposable : IDisposable
    {
    }
}

Here is how usage might look:

var clock = new VirtualClock(new DateTime(2024, 2, 29));
using (TimeContext.WithClock(clock) {
    var dt = DateTime.Now;      // 2024-02-29T00:00:00
};
var initialValue = new DateTimeOffset(1999, 12, 31, 23, 59, 59, TimeSpan.Zero);
var advancementAmount = TimeSpan.FromSeconds(1);
var clock = new VirtualClock(initialValue, advancementAmount);
using (TimeContext.WithClock(clock) {
    var dt1 = DateTime.UtcNow;  // 1999-12-31T23:59:59Z
    var dt2 = DateTime.UtcNow;  // 2000-01-01T01:00:00Z
};
var initialValue = new DateTime(2020, 11, 1);
var advancementAmount = TimeSpan.FromHours(1);
TimeClock clock = new VirtualClock(initialValue, advancementAmount);
TimeZoneInfo tz = TimeZoneInfo.FindSystemTimeZoneById("Central Standard Time"); // "America/Chicago" on Linux
using (TimeContext.WithClockAndTimeZone(clock, tz) {
    var dto1 = DateTimeOffset.Now;  // 2020-11-01T00:00:00-05:00 (CDT)
    var dto2 = DateTimeOffset.Now;  // 2020-11-01T01:00:00-05:00 (CDT)
    var dto3 = DateTimeOffset.Now;  // 2020-11-01T01:00:00-06:00 (CST)
    var dto4 = DateTimeOffset.Now;  // 2020-11-01T02:00:00-06:00 (CST)
};

As you can see not so much changed, just a bit more wordy method name and less braces, but with that approach there will be much less memory allocations involved.

ChadNedzlek commented 3 years ago

Calling it "TimeClock" is a little weird, because it's got date information in it... and "TimeClock" feels like, well, a time clock on the wall... a thing that doesn't have a date component. Though "ISystemClock" isn't much better in that regard, at least it doesn't reinforce the "Time" part of it, when the date part is equally important. "DateTime" and "DateTimeOffset" both include the fact that dates are involved (They could have just been "Time" and "TimeOffset"), so it feels like this concept, if the word "Time" is going to be used, the word "Date" needs to be too.

ChadNedzlek commented 3 years ago

It's also a bit surprising that the behavior of the property accessors on DateTime and DateTimeOffset are going to change after so long. That's a fairly significant behavior change to some pretty core functionality in .NET. Maybe that's ok? I'd personally probably never use that side effecting behavior: I'd make sure any code that wanted to be sensitive to time contexts always got one of these objects from somewhere. I would be worried about third party libraries I'm using failing in strange ways if the behavior of DateTime.Now changed out from under them.

mattjohnsonpint commented 3 years ago

Thanks very much for the feedback thus far.

@ChadNedzlek - I would also be ok with DateTimeClock, or just Clock. Naming is hard. 😉 Also, the point you made about a library's behavior changing is exactly why having a TimeContext is valuable. If one is intending to test the behavior of application code, and that code uses a library that in turn gets the current time, then there's not going to be a direct way to provide a clock unless the library author decides to expose some way to pass one in. Since most code just uses DateTime.Now and its friends, we need some ability to have that emit test values within an outer scope.

@YohDeadfall - I like your idea about using a lightweight struct to avoid allocations. However I still have concerns about the disposable scope around what happens if it's not disposed correctly, and when code falls between when the context is declared and the using statement. Perhaps an analyzer would be helpful to ensure correct usage there?

I had one other idea I haven't explored yet, but would like to discuss. Since there's some concern about how the TimeContext might be abused (say a library author uses TimeContext in the library's code instead of in a test), we could add some application configuration flags that would have to be set to be able to detour the clock or local time zone. This would be very similar to how one can set the runtime host configuration to use NLS instead of ICU. There would be two new boolean flags, something like AllowCurrentDateTimeFromTimeContext and AllowLocalTimeZoneFromTimeContext that would have to be set true for the context to have any effect (the default being false). These would then be set true on unit test projects that wanted to take advantage of this feature, or perhaps automatically set true by test runners. This would also solve some of the perf concerns, as this would become an opt-in feature. Thoughts?

FiniteReality commented 3 years ago

say a library author uses TimeContext in the library's code instead of in a test

It may be advantageous to use a hypothetical TimeContext to affect ambient code to reflect a user's timezone, similar to how ASP.NET's RequestLocalizationMiddleware updates CultureInfo.CurrentCulture to enable localization.

mattjohnsonpint commented 3 years ago

@FiniteReality - Indeed! Such an ASP.net project could have AllowLocalTimeZoneFromTimeContext turned on in their runtime config, yes?

FiniteReality commented 3 years ago

@FiniteReality - Indeed! Such an ASP.net project could have AllowLocalTimeZoneFromTimeContext turned on in their runtime config, yes?

Possibly - I'd assume such a change would have a higher barrier to entry though, since it would take a while to become available everywhere for ASP.NET to be able to use it

YohDeadfall commented 3 years ago

However I still have concerns about the disposable scope around what happens if it's not disposed correctly, and when code falls between when the context is declared and the using statement. Perhaps an analyzer would be helpful to ensure correct usage there?

Yes, it should be covered by an analyzer that any disposable is actually disposed. As I remember JetBrain Rider highlights all such places and notify the developer than that should be fixed. While some code (read SQL Client) allows abandoning disposable objects most of .NET ecosystem doesn't support that.

ChadNedzlek commented 3 years ago

I can see the compat breaking change of how DateTime works causing some really hard to diagnose problems. Like a caching library with a background "cleanup" thread. That code isn't going to expect "UtcNow" to change so drastically between contexts (because it's been stable for a few decades). If anyone uses the static altering TimeContext, things could go badly (things either never expiring, or instantly expiring). And that caching might be 4 levels deep in a dependency tree, so you, the user of TimeContext, have no way to know if you're going to break something... because it's static, and static state changes are impossible to judge the scope of. It's not even really something that library could have been hardened against, because this behavior isn't possible today.

Maybe that means "Ok, only use it for testing, never in real code", but that's an odd design choice for something in the framework. Something that is dangerous enough that needs recommendations not to use it in "real" code.

I'm happy about having a clock someone can request and use, meaning that code has explicitly stated their presumed understanding that this clock has context associated with it. So DateTimeClock.Current is fine. It's changing the very old behavior of DateTime.Now that worries me the most.

ChadNedzlek commented 3 years ago

The original proposal (perhaps with the few additional method suggested later) seems like a safe, cheap, understandable solution, without any of the compatibility problems, that solves a very large number of scenarios. In particular, even if this revised proposal were accepted, I would still continue to use Microsoft.Extensions.Internal.ISystemClock in all my code.

Clockwork-Muse commented 3 years ago

I personally, regardless of slight performance hits, vastly prefer explicitly providing some sort of IClock interface to anything needing to get time-related information. Time is one of the more insidious system dependencies, and I like to be explicit that it's being used.

Note that, for most applications, there will be exactly one clock for the runtime of an application, instantiated during initialization, and the specific class known at compile time.

AraHaan commented 3 years ago

However I still have concerns about the disposable scope around what happens if it's not disposed correctly, and when code falls between when the context is declared and the using statement. Perhaps an analyzer would be helpful to ensure correct usage there?

Yes, it should be covered by an analyzer that any disposable is actually disposed. As I remember JetBrain Rider highlights all such places and notify the developer than that should be fixed. While some code (read SQL Client) allows abandoning disposable objects most of .NET ecosystem doesn't support that.

That is IDisposableAnalyzers you are asking for (which is outside the scope of this issue).

dazinator commented 3 years ago

I think if there was a primitive for ISystemClock that all .NET 6 developers who care about mocking datetime's could agree on, you'd see code bases migrate to it over time rather than roll their own. An optional analyzer could detect accesses of DateTime and friends outside of an ISystemClock implementation and say "hey have you considered using ISystemClock for better mockability" or something similar. I can see the appeal of changing the behaviour of DateTime and friends within a given context using the awesome soluton above, but that also makes me very nervous around edge cases where third party libraries undergo time travel at runtime in ways they might not have expected - perhaps that caution is unjustified though :-). Also what about nested contexts - would that be a thing?

kaylumah commented 3 years ago

@dazinator I like that addition to the proposal, it would help developers move away from statics in their code base

julealgon commented 3 years ago

I noticed a surprising lack of discussion around the name of the abstraction itself. While ISystemClock is now present in a bunch of different places, I'm not convinced it is a good name at all, to be honest. I'd even go as far as to say that it has this name in multiple places due to influence from the originator and was just replicated as-is later.

First, "System" in the name to me implies a concrete implementation already. The fact it provides the "system"'s clock and not "another clock" is an implementation detail that should not be in the interface name itself. Thus, IClock would make more sense for this aspect.

Having said that, "clocks" don't usually provide date information, only time of day. With that in mind, I think using clock (in either IClock or ISystemClock forms) is already misleading conceptually. I know this is can be a bit silly, but perhaps something more direct and obvious, like IDateTimeProvider, would be more semantic and match existing DateTime types? In that case, you'd then have a SystemDateTimeProvider concrete implementation which is the wrapper that delegates to the static method (again, System here being the implementation detail).

I'm not convinced that IDateTimeProvider is a perfect name. I just think ISystemClock is a fairly bad one.

Clockwork-Muse commented 3 years ago

One thing to note: Outside of the current instant, it can be incredibly useful to also provide the "current" timezone, especially for something like a server web application (it provides the user's "local" clock).

public interface ISystemClock
{
    DateTimeOffset UtcNow { get; }
    TimeZoneInfo LocalZone { get; }
    // also include?
    DateTimeOffset LocalNow => TimeZoneInfo.ConvertTime(UtcNow, CurrentZone);
}

For tests, as well, this would free us from having to rely on system-provided zones at all, since you could manually construct a number of them ahead of time to use for the clock.

ChadNedzlek commented 3 years ago

I feel like the "System" part is the implication that it's only job is to return what time the system thinks it is. And, indeed, that's what the interface does (it only has "now" type properties). An IDateTimeProvider doesn't have the same implicit "this is all about now", and feels like it might provide arbitrary times, which isn't really what the abstraction is about. INowProvider is... weird.

Clockwork-Muse commented 3 years ago

An IDateTimeProvider doesn't have the same implicit "this is all about now", and feels like it might provide arbitrary times, which isn't really what the abstraction is about.

To an extent it is, actually. Besides testing, it might be handy to provide a "future time" clock for certain simulation/scheduling scenarios, and it makes fun use-cases like time dilation or contraction much easier.

ChadNedzlek commented 3 years ago

I think that's a very different object. If I'm looking at code that's using an ISystemClock, I understand based on that that the code is just trying to deal with the current time, which is useful information. If it's just random, arbitrary times, the usage will have to look very different, and that would be important to know, so it should probably be a separate abstraction.

dazinator commented 3 years ago

An IDateTimeProvider doesn't have the same implicit "this is all about now"

ICurrentDateTimeProvider ?

Clock is more concise than CurrentDateTimeProvider though.. as long as you can glance past the point about a clocks purpose not providing dates but only time information. Some digital clocks do have supplementary date information, it wouldn't keep me awake at night personally.

FiniteReality commented 3 years ago

ISystemClock seems like a fine name to me:

julealgon commented 3 years ago

What about the property name itself? Similarly to my argument on the class name, I think the Utc part is an implementation detail.

Instead of 

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

Wouldn't it be better to have:

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

?

You could then potentially have 2 distinct implementations: one that returns UTC, and the other that returns the adjusted time:

public class SystemClock : ISystemClock
{
    public DateTimeOffset Now => DateTimeOffset.Now;
}
Clockwork-Muse commented 3 years ago

... NodaTime and Java get around this by having an explicit absolute timestamp type (something without a zone attached at all).

I personally feel that it's better to have something that explicitly returns a UTC (or some zone-ignorant) stamp + a zone, as both NodaTime and Java do, and maybe including an additional method/property that gets the local time. You cannot avoid including the zone, I don't think, because otherwise you lose too much context (for one thing, "did DST just occur?").