4.x: Fix PeriodicTimerSystemClockMonitor concurrency & failure behavior

[akarnokd]

This PR fixes the PeriodicTimerSystemClockMonitor to:

• avoid restarting the timer when a new listener joins,
• avoid stopping the timer when one listener leaves (and there could be others still listening),
• avoid the data races between the NewTimer and the TimerChanged which involves using unix time milliseconds for the absolute time that can be atomically manipulated via Volatile/Interlocked,
• do not fail if the SYNC_MAXRETRIES has been reached but start backing off and sleeping a bit before retrying.

Fixes: #270

[danielcweber]

avoid restarting the timer when a new listener joins

This probably is not really what happens since there is AddRef and Release in SystemClock, although it should definitely be handled by the PeriodicTimerSystemClockMonitor.

[danielcweber]

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?

[akarnokd]

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.

[akarnokd]

This probably is not really what happens since there is AddRef and Release in SystemClock

Indeed, interesting. Kind of hard to determine the intention here since the class itself is public and the notification is designed with an event: both implying the feature could be used from the outside and perhaps on multiple occasions. I'd say the safety should be added because of it.

[danielcweber]

I'd say the safety should be added because of it.

Definitely, but can we get rid of AddRef and Release then?

[akarnokd]

I don't think so. If removed, it would do this all the time:

s_serviceSystemClockChanged.Value.SystemClockChanged += OnSystemClockChanged;

potentially registering that method over and over resulting in duplicate notifications.

[danielcweber]

This isn't thread safe, if we deal with Interlocked above, this all should go into a lock then.

[akarnokd]

I though += on a delegate is generates as an atomic operation by the compiler.

[danielcweber]

Nope, this is just non atomic read, change and write operation on a field.

[akarnokd]

I haven't changed that line so if that's true, you've been running with an unsafe add for a long time now. Or, the compiler is smart enough to produce += as an atomic exchange:

https://stackoverflow.com/a/3522556/61158

[danielcweber]

The compiler would do magic if the event add/remove code wasn't explicitly implemented. I think the unsafe add is harmless since that class only has only one consumer anyway. That being said, I agree that all of that looks horribly broken.

[danielcweber]

It's really odd, what this code does (creating a resource on first subscribe etc) is solvable easily by Rx itself! It should be redone with Rx.

[akarnokd]

It would create a cyclic dependency with the standard Rx operators. What if you'd want to subscribe to this after some time delay, which would require this to be setup already...

[danielcweber]

Can you revert the atomic counting in the event, the class and events are not thread safe even without, and I guess they don't have to be. I'll merge this PR then.

[danielcweber]

I think an oddity here is that the code could perfectly well use Rx itself...FromEvent, RefCount, all that stuff.

[akarnokd]

I think there would be a chance of circular dependency when schedulers come into play.