Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Apply the TimeProvider design feedback #84501

Merged
merged 4 commits into from
Apr 9, 2023
Merged

Apply the TimeProvider design feedback #84501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d20a959
Apply the TimeProvider design feedback
tarekgh Apr 7, 2023
8cf4c07
Address the feedback
tarekgh Apr 8, 2023
e9b6e69
Fix the exception thrown because of the timestamp frequency
tarekgh Apr 8, 2023
df6e180
Fix exception message.
tarekgh Apr 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
293 changes: 136 additions & 157 deletions src/libraries/Common/src/System/TimeProvider.cs
View file
Open in desktop

Large diffs are not rendered by default.

61 changes: 37 additions & 24 deletions src/libraries/Common/tests/System/TimeProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class TimeProviderTests
public void TestUtcSystemTime()
{
DateTimeOffset dto1 = DateTimeOffset.UtcNow;
DateTimeOffset providerDto = TimeProvider.System.UtcNow;
DateTimeOffset providerDto = TimeProvider.System.GetUtcNow();
DateTimeOffset dto2 = DateTimeOffset.UtcNow;

Assert.InRange(providerDto.Ticks, dto1.Ticks, dto2.Ticks);
Expand All @@ -28,7 +28,7 @@ public void TestUtcSystemTime()
public void TestLocalSystemTime()
{
DateTimeOffset dto1 = DateTimeOffset.Now;
DateTimeOffset providerDto = TimeProvider.System.LocalNow;
DateTimeOffset providerDto = TimeProvider.System.GetLocalNow();
DateTimeOffset dto2 = DateTimeOffset.Now;

// Ensure there was no daylight saving shift during the test execution.
Expand All @@ -39,6 +39,17 @@ public void TestLocalSystemTime()
}
}

private class ZonedTimeProvider : TimeProvider
{
private TimeZoneInfo _zoneInfo;
public ZonedTimeProvider(TimeZoneInfo zoneInfo) : base()
{
_zoneInfo = zoneInfo ?? TimeZoneInfo.Local;
}
public override TimeZoneInfo LocalTimeZone { get => _zoneInfo; }
public static TimeProvider FromLocalTimeZone(TimeZoneInfo zoneInfo) => new ZonedTimeProvider(zoneInfo);
}

[Fact]
public void TestSystemProviderWithTimeZone()
{
Expand All @@ -50,11 +61,11 @@ public void TestSystemProviderWithTimeZone()
TimeZoneInfo tzi = TimeZoneInfo.FindSystemTimeZoneById(OperatingSystem.IsWindows() ? "Pacific Standard Time" : "America/Los_Angeles");
#endif // NETFRAMEWORK

TimeProvider tp = TimeProvider.FromLocalTimeZone(tzi);
TimeProvider tp = ZonedTimeProvider.FromLocalTimeZone(tzi);
Assert.Equal(tzi.Id, tp.LocalTimeZone.Id);

DateTimeOffset utcDto1 = DateTimeOffset.UtcNow;
DateTimeOffset localDto = tp.LocalNow;
DateTimeOffset localDto = tp.GetLocalNow();
DateTimeOffset utcDto2 = DateTimeOffset.UtcNow;

DateTimeOffset utcConvertedDto = TimeZoneInfo.ConvertTime(localDto, TimeZoneInfo.Utc);
Expand Down Expand Up @@ -139,12 +150,12 @@ public void FastClockTest()

for (int i = 0; i < 20; i++)
{
DateTimeOffset fastNow = fastClock.UtcNow;
DateTimeOffset fastNow = fastClock.GetUtcNow();
DateTimeOffset now = DateTimeOffset.UtcNow;

Assert.True(fastNow > now, $"Expected {fastNow} > {now}");

fastNow = fastClock.LocalNow;
fastNow = fastClock.GetLocalNow();
now = DateTimeOffset.Now;

Assert.True(fastNow > now, $"Expected {fastNow} > {now}");
Expand Down Expand Up @@ -375,9 +386,10 @@ public static async void PeriodicTimerTests(TimeProvider provider)
[Fact]
public static void NegativeTests()
{
Assert.Throws<ArgumentOutOfRangeException>(() => new FastClock(-1)); // negative frequency
Assert.Throws<ArgumentOutOfRangeException>(() => new FastClock(0)); // zero frequency
Assert.Throws<ArgumentNullException>(() => TimeProvider.FromLocalTimeZone(null));
FastClock clock = new FastClock(-1); // negative frequency
Assert.Throws<InvalidOperationException>(() => clock.GetElapsedTime(1, 2));
clock = new FastClock(0); // zero frequency
Assert.Throws<InvalidOperationException>(() => clock.GetElapsedTime(1, 2));

Assert.Throws<ArgumentNullException>(() => TimeProvider.System.CreateTimer(null, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan));
Assert.Throws<ArgumentOutOfRangeException>(() => TimeProvider.System.CreateTimer(obj => { }, null, TimeSpan.FromMilliseconds(-2), Timeout.InfiniteTimeSpan));
Expand Down Expand Up @@ -414,34 +426,35 @@ class FastClock : TimeProvider
{
private long _minutesToAdd;
private TimeZoneInfo _zone;
private long _timestampFrequency;

public FastClock(long timestampFrequency = TimeSpan.TicksPerSecond, TimeZoneInfo? zone = null) : base(timestampFrequency)
public FastClock(long timestampFrequency = TimeSpan.TicksPerSecond, TimeZoneInfo? zone = null) : base()
{
_timestampFrequency = timestampFrequency;
_zone = zone ?? TimeZoneInfo.Local;
}

public override DateTimeOffset UtcNow
public override DateTimeOffset GetUtcNow()
{
get
{
DateTimeOffset now = DateTimeOffset.UtcNow;

_minutesToAdd++;
long remainingTicks = (DateTimeOffset.MaxValue.Ticks - now.Ticks);
DateTimeOffset now = DateTimeOffset.UtcNow;

if (_minutesToAdd * TimeSpan.TicksPerMinute > remainingTicks)
{
_minutesToAdd = 0;
return now;
}
_minutesToAdd++;
long remainingTicks = (DateTimeOffset.MaxValue.Ticks - now.Ticks);

return now.AddMinutes(_minutesToAdd);
if (_minutesToAdd * TimeSpan.TicksPerMinute > remainingTicks)
{
_minutesToAdd = 0;
return now;
}

return now.AddMinutes(_minutesToAdd);
}

public override long TimestampFrequency { get => _timestampFrequency; }

public override TimeZoneInfo LocalTimeZone => _zone;

public override long GetTimestamp() => UtcNow.Ticks;
public override long GetTimestamp() => GetUtcNow().Ticks;

public override ITimer CreateTimer(TimerCallback callback, object? state, TimeSpan dueTime, TimeSpan period) =>
new FastTimer(callback, state, dueTime, period);
Expand Down
15 changes: 7 additions & 8 deletions src/libraries/Microsoft.Bcl.TimeProvider/ref/Microsoft.Bcl.TimeProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,14 @@ namespace System
public abstract class TimeProvider
{
public static TimeProvider System { get; }
protected TimeProvider(long timestampFrequency) { throw null; }
public abstract System.DateTimeOffset UtcNow { get; }
public System.DateTimeOffset LocalNow { get; }
public abstract System.TimeZoneInfo LocalTimeZone { get; }
public long TimestampFrequency { get; }
public static TimeProvider FromLocalTimeZone(System.TimeZoneInfo timeZone) { throw null; }
public abstract long GetTimestamp();
protected TimeProvider() { throw null; }
public virtual System.DateTimeOffset GetUtcNow() { throw null; }
public System.DateTimeOffset GetLocalNow() { throw null; }
public virtual System.TimeZoneInfo LocalTimeZone { get; }
public virtual long TimestampFrequency { get; }
public virtual long GetTimestamp() { throw null; }
public TimeSpan GetElapsedTime(long startingTimestamp, long endingTimestamp) { throw null; }
public abstract System.Threading.ITimer CreateTimer(System.Threading.TimerCallback callback, object? state, System.TimeSpan dueTime, System.TimeSpan period);
public virtual System.Threading.ITimer CreateTimer(System.Threading.TimerCallback callback, object? state, System.TimeSpan dueTime, System.TimeSpan period) { throw null; }
Copy link

@niemyjski niemyjski Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Why would you want to throw null here? I'd expect NotImplementedException / abstract implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reference assembly. It should never be used at runtime, and the goal is to define just the signatures while keeping the size as small as possible. throw null is what we do in the ref assemblies everywhere.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense!

}
}
namespace System.Threading
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/Microsoft.Bcl.TimeProvider/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,10 @@
<data name="ArgumentOutOfRange_Generic_MustBeLessOrEqual" xml:space="preserve">
<value>'{0}' must be less than or equal to '{1}'.</value>
</data>
<data name="InvalidOperation_TimeProviderNullLocalTimeZone" xml:space="preserve">
<value>The operation cannot be performed when TimeProvider.LocalTimeZone is null.</value>
</data>
<data name="InvalidOperation_TimeProviderInvalidTimestampFrequency" xml:space="preserve">
<value>The operation cannot be performed when TimeProvider.TimestampFrequency is zero or negative.</value>
</data>
</root>
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2984,6 +2984,12 @@
<data name="InvalidOperation_SpanOverlappedOperation" xml:space="preserve">
<value>This operation is invalid on overlapping buffers.</value>
</data>
<data name="InvalidOperation_TimeProviderNullLocalTimeZone" xml:space="preserve">
<value>The operation cannot be performed when TimeProvider.LocalTimeZone is null.</value>
</data>
<data name="InvalidOperation_TimeProviderInvalidTimestampFrequency" xml:space="preserve">
<value>The operation cannot be performed when TimeProvider.TimestampFrequency is zero or negative.</value>
</data>
<data name="NotSupported_IDispInvokeDefaultMemberWithNamedArgs" xml:space="preserve">
<value>Invoking default method with named arguments is not supported.</value>
</data>
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,10 @@ private static string GetResourceString(ExceptionResource resource)
return SR.ArgumentOutOfRange_NotGreaterThanBufferLength;
case ExceptionResource.InvalidOperation_SpanOverlappedOperation:
return SR.InvalidOperation_SpanOverlappedOperation;
case ExceptionResource.InvalidOperation_TimeProviderNullLocalTimeZone:
return SR.InvalidOperation_TimeProviderNullLocalTimeZone;
case ExceptionResource.InvalidOperation_TimeProviderInvalidTimestampFrequency:
return SR.InvalidOperation_TimeProviderInvalidTimestampFrequency;
default:
Debug.Fail("The enum value is not defined, please check the ExceptionResource Enum.");
return "";
Expand Down Expand Up @@ -1285,5 +1289,7 @@ internal enum ExceptionResource
CancellationTokenSource_Disposed,
Argument_AlignmentMustBePow2,
InvalidOperation_SpanOverlappedOperation,
InvalidOperation_TimeProviderNullLocalTimeZone,
InvalidOperation_TimeProviderInvalidTimestampFrequency,
}
}
15 changes: 7 additions & 8 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1854,15 +1854,14 @@ public enum DayOfWeek
public abstract class TimeProvider
{
public static TimeProvider System { get; }
protected TimeProvider(long timestampFrequency) { throw null; }
public abstract System.DateTimeOffset UtcNow { get; }
public System.DateTimeOffset LocalNow { get; }
public abstract System.TimeZoneInfo LocalTimeZone { get; }
public long TimestampFrequency { get; }
public static TimeProvider FromLocalTimeZone(System.TimeZoneInfo timeZone) { throw null; }
public abstract long GetTimestamp();
protected TimeProvider() { throw null; }
public virtual System.DateTimeOffset GetUtcNow() { throw null; }
public System.DateTimeOffset GetLocalNow() { throw null; }
public virtual System.TimeZoneInfo LocalTimeZone { get; }
public virtual long TimestampFrequency { get; }
public virtual long GetTimestamp() { throw null; }
public TimeSpan GetElapsedTime(long startingTimestamp, long endingTimestamp) { throw null; }
public abstract System.Threading.ITimer CreateTimer(System.Threading.TimerCallback callback, object? state, System.TimeSpan dueTime, System.TimeSpan period);
public virtual System.Threading.ITimer CreateTimer(System.Threading.TimerCallback callback, object? state, System.TimeSpan dueTime, System.TimeSpan period) { throw null; }
}
public sealed partial class DBNull : System.IConvertible, System.Runtime.Serialization.ISerializable
{
Expand Down