Skip to content

4.x: Fix PeriodicTimerSystemClockMonitor concurrency & failure behavior #528

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

Merged
danielcweber merged 3 commits into from
Jun 5, 2018
Merged

4.x: Fix PeriodicTimerSystemClockMonitor concurrency & failure behavior #528

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88979ff
4.x: Fix PeriodicTimerSystemClockMonitor concurrency & failure behavior
akarnokd May 25, 2018
2b0f8ce
Revert to non-threadsafe registration
akarnokd Jun 5, 2018
ba83352
Merge branch 'master' into PeriodicTimerSystemClockMonitorFix
akarnokd Jun 5, 2018
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
48 changes: 36 additions & 12 deletions Rx.NET/Source/src/System.Reactive/Internal/SystemClock.Default.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
using System.ComponentModel;
using System.Reactive.Concurrency;
using System.Reactive.Disposables;
using System.Threading;
using System.Threading.Tasks;

namespace System.Reactive.PlatformServices
{
Expand Down Expand Up @@ -39,7 +41,12 @@ public class PeriodicTimerSystemClockMonitor : INotifySystemClockChanged
private readonly TimeSpan _period;
private readonly SerialDisposable _timer;

private DateTimeOffset _lastTime;
/// <summary>
/// Use the Unix milliseconds for the current time
/// so it can be atomically read/written without locking.
/// </summary>
private long _lastTimeUnixMillis;

private EventHandler<SystemClockChangedEventArgs> _systemClockChanged;

private const int SYNC_MAXRETRIES = 100;
Expand Down Expand Up @@ -80,30 +87,47 @@ private void NewTimer()
{
_timer.Disposable = Disposable.Empty;

var n = 0;
do
var n = 0L;
for (; ; )
{
_lastTime = SystemClock.UtcNow;
var now = SystemClock.UtcNow.ToUnixTimeMilliseconds();
Interlocked.Exchange(ref _lastTimeUnixMillis, now);

_timer.Disposable = ConcurrencyAbstractionLayer.Current.StartPeriodicTimer(TimeChanged, _period);
} while (Math.Abs((SystemClock.UtcNow - _lastTime).TotalMilliseconds) > SYNC_MAXDELTA && ++n < SYNC_MAXRETRIES);

if (n >= SYNC_MAXRETRIES)
throw new InvalidOperationException(Strings_Core.FAILED_CLOCK_MONITORING);
if (Math.Abs(SystemClock.UtcNow.ToUnixTimeMilliseconds() - now) <= SYNC_MAXDELTA)
Copy link
Collaborator

@danielcweber danielcweber May 25, 2018

Choose a reason for hiding this comment

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

Why does a new timer have to be created, just because the previous one took to long to create? Can't we deal with that otherwise?

Copy link
Collaborator Author

@akarnokd akarnokd May 25, 2018

Choose a reason for hiding this comment

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

This is my understanding of the original logic:

The logic saves the Now in the _lastTime field and when the scheduled task runs, it reads it out to determine how much time has passed since the start of the timer and the first tick of the timer. If, for some reason the start itself gets delayed and runs eventually, you may end up with a clock-drift notification right away. Instead, the previous timer is cancelled and a new round is attempted. I'd think when the drift does actually happen this very first time, the timer action will restart the timer anyway so my guess is that this just makes the syncing happen earlier.

{
break;
}
if (_timer.Disposable == Disposable.Empty)
{
break;
}
if (++n >= SYNC_MAXRETRIES)
{
Task.Delay((int)SYNC_MAXDELTA).Wait();
}
};
}

private void TimeChanged()
{
var now = SystemClock.UtcNow;
var diff = now - (_lastTime + _period);
if (Math.Abs(diff.TotalMilliseconds) >= MAXERROR)
var newTime = SystemClock.UtcNow;
var now = newTime.ToUnixTimeMilliseconds();
var last = Volatile.Read(ref _lastTimeUnixMillis);

var oldTime = (long)(last + _period.TotalMilliseconds);
var diff = now - oldTime;
if (Math.Abs(diff) >= MAXERROR)
{
_systemClockChanged?.Invoke(this, new SystemClockChangedEventArgs(_lastTime + _period, now));
_systemClockChanged?.Invoke(this, new SystemClockChangedEventArgs(
DateTimeOffset.FromUnixTimeMilliseconds(oldTime), newTime));

NewTimer();
}
else
{
_lastTime = SystemClock.UtcNow;
Interlocked.Exchange(ref _lastTimeUnixMillis, SystemClock.UtcNow.ToUnixTimeMilliseconds());
}
}
}
Expand Down