Address the TimeProvider community feedback

[tarekgh]

#36617

Original writing by @stephentoub

• 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

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:

Change Proposal

namespace System
{
    public abstract class TimeProvider
    {
        public static TimeProvider System { get; }
        
+      protected TimeProvider();

-      public abstract DateTimeOffset UtcNow { get; }
+      public virtual DateTimeOffset GetUtcNow();

-      public DateTimeOffset LocalNow { get; }
+      public DateTimeOffset GetLocalNow();

-      public abstract TimeZoneInfo LocalTimeZone { get; }
+      public virtual TimeZoneInfo LocalTimeZone { get; }

-      public abstract long GetTimestamp();
+      public virtual long GetTimestamp();

-      protected TimeProvider(long timestampFrequency);
-      public static TimeProvider FromLocalTimeZone(TimeZoneInfo timeZone);

-      public long TimestampFrequency { get; }
+      public virtual long TimestampFrequency { get; } // or `public long TimestampFrequency { get; }` and `protected virtual long TimestampFrequencyCore { get; }`

-      public abstract ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
+      public virtual ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);

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

[ghost]

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

#36617

Original writing by @stephentoub

• 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:

Change Proposal

namespace System
{
    public abstract class TimeProvider
    {
        public static TimeProvider System { get; }
        
+      protected TimeProvider();

-      public abstract DateTimeOffset UtcNow { get; }
+      public virtual DateTimeOffset GetUtcNow();

-      public DateTimeOffset LocalNow { get; }
+      public DateTimeOffset GetLocalNow();

-      public abstract TimeZoneInfo LocalTimeZone { get; }
+      public virtual TimeZoneInfo LocalTimeZone { get; }

-      public abstract long GetTimestamp();
+      public virtual long GetTimestamp();

-      protected TimeProvider(long timestampFrequency);
-      public static TimeProvider FromLocalTimeZone(TimeZoneInfo timeZone);

-      public long TimestampFrequency { get; }
+      public virtual long TimestampFrequency { get; } // or `public long TimestampFrequency { get; }` and `protected virtual long TimestampFrequencyCore { get; }`

-      public abstract ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
+      public virtual ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);

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

Author:	tarekgh
Assignees:	-
Labels:	area-System.DateTime
Milestone:	-

[tarekgh]

CC @davidfowl @geeknoid

[terrajobst]

Video

• Looks good as proposed

 namespace System;

 public abstract class TimeProvider
 {
     public static TimeProvider System { get; }
     
+    protected TimeProvider();
 
-    public abstract DateTimeOffset UtcNow { get; }
+    public virtual DateTimeOffset GetUtcNow();
 
-    public DateTimeOffset LocalNow { get; }
+    public DateTimeOffset GetLocalNow();
 
-    public abstract TimeZoneInfo LocalTimeZone { get; }
+    public virtual TimeZoneInfo LocalTimeZone { get; }
 
-    public abstract long GetTimestamp();
+    public virtual long GetTimestamp();
 
-    protected TimeProvider(long timestampFrequency);
-    public static TimeProvider FromLocalTimeZone(TimeZoneInfo timeZone);
 
-    public long TimestampFrequency { get; }
+    public virtual long TimestampFrequency { get; }
 
-    public abstract ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
+    public virtual ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period);
 
     public TimeSpan GetElapsedTime(long startingTimestamp, long endingTimestamp);
 }

[wagich]

It seems a bit odd to me to dismiss the case of a TimeProvider with a time zone other than that of the system as niche…
There seem to be many scenarios where the user time zone is known – maybe it's a local audience or even a whole (european) country – but where the server time zone differs from the user.

F.ex. I'm hosting quite a few websites on Azure App Service where the server time zone is set to UTC and it'd be much simpler if I could just put the result of TimeProvider.FromLocalTimeZone(europeZurichTz) into DI and have a useful local date/time instead of always having to convert before doing anything.
Sure, subclassing is easy as mentioned, but thats one more thing that needs to be thought of and added into every project.

To avoid exposing a static factory, maybe the proposed LocalTimeProvider could also be included by default?

[Clockwork-Muse]

DI and have a useful local date/time instead

Nit: in most cases the zone of the server process isn't relevant either, and you should be pulling the zone from a business record or profile data, even if you essentially have a "constant" zone.

[terrajobst]

@wagich

It seems a bit odd to me to dismiss the case of a TimeProvider with a time zone other than that of the system as niche

That's fair, but I'd like to point out that this doesn't mean that you can't get the current time with a different timezone, just that you need to translate it yourself, which matches what you'd do today:

TimeZoneInfo tz = /*...*/;

DateTimeOffset staticWay = TimeZoneInfo.ConvertTime(DateTimeOffset.Now, tz);
DateTimeOffset timeProviderWay = TimeZoneInfo.ConvertTime(timeProvider.GetLocalNow(), tz);

If we believe this is a common enough problem, we can bring back a static factory.

To avoid exposing a static factory, maybe the proposed LocalTimeProvider could also be included by default?

From my point of view, I'd rather have one more method than exposing another type.

@Clockwork-Muse

Nit: in most cases the zone of the server process isn't relevant either, and you should be pulling the zone from a business record or profile data, even if you essentially have a "constant" zone.

Agreed. I would think that the server mostly deals with UTC. Aren't most cloud machines configured to have UTC as their local time zone? My understanding is that time zone is mostly a client thing; data should be stored in UTC and converted on the client. There are of course exceptions, like a calendar application might allow appointments to be configured to be for a certain time zone. But even in that case I'd think the date fields would be stored as UTC and the time zone be stored as a separate field.

[Clockwork-Muse]

If we believe this is a common enough problem, we can bring back a static factory.

Note that I'm actually arguing for this method, but arguing against using it at server start-time DI. You potentially want to use it in request time DI, as part of entity hydration (for example when user records are hydrated to provide roles, etc).

Agreed. I would think that the server mostly deals with UTC. Aren't most cloud machines configured to have UTC as their local time zone?

Servers being set to UTC is a "solution domain" problem - that is, you set it to UTC because it has to be set to something, but pedantically the fact that it's in a "timezone" should be irrelevant in the first place. Some cloud services do allow you to set a timezone, which might be required in some cases - the most relevant example I've seen is for Azure FaaS, with a scheduled trigger.

data should be stored in UTC and converted on the client.

This is false and true.

Storing in UTC is a "solution domain" issue, in that you don't actually care about UTC at all, it's just what most platforms/languages provide to you. The more relevant type is equivalent to the Java Instant type (which is a seconds-since timestamp). For logged events, it usually makes sense to store data this way (there are some cases where for faster reporting reasons you might want to store it in the offset of the reporting entity, though).

Yes, you would convert on the client. That said, you have two potential timezones to consider:

• The zone of the reporting entity.
• The zone of the person viewing the event.
  Which one you want depends, and you may need both in some circumstances (and potentially also want the neutral stamp too in some cases).

There are of course exceptions, like a calendar application might allow appointments to be configured to be for a certain time zone. But even in that case I'd think the date fields would be stored as UTC and the time zone be stored as a separate field.

Although some people argue for this, I'm in agreement with Jon Skeet's views on this, which advocates for storing local time, due to problems with updating timezone rules.