RUMM-1609 Use NTP server offset

[maxep]

What and why?

The Kronos Clock.now property is not thread safe and should always be invoked from the main thread, this change would have a large impact on our codebase. As an alternative, we can rely on the server time offset we get from Clock.sync: The Clock.now is in fact used by DateCorrector to actually compute the server offset.

Below a quick benchmark of the computed offset previously used and the server offset applied by this PR:

Previous Computed Offset	Server Offset
0.053711891174316406	0.05375981330871582
0.04763531684875488	0.04767334461212158
0.04572319984436035	0.045766234397888184
0.08127260208129883	0.08131551742553711
0.05130136013031006	0.051299452781677246
0.03885924816131592	0.038874030113220215

Note that Kronos.Clock.now has a sub-second precision which is reflected in the above tests.

Fix #588

How?

The ServerDateProvider will now provide an offset time interval and its concrete implementation NTPServerDateProvider will retrieve that value from Kronos.Clock.sync.

Clock.sync callback is invoked on the main thread, so we are now using a ValuePublisher to safely access the server time offset.

⚠️ Clock.sync only notifies the first and last samples, so we are losing the intermediate offsets!

Review checklist

• Feature or bugfix MUST have appropriate tests (unit, integration)
• Make sure each commit and the PR mention the Issue number or JIRA reference

[ncreated]

⚠️ Clock.sync only notifies the first and last samples, so we are losing the intermediate offsets!

Is it true that previously (with using Clock.now) we were receiving some initial approximation even before Clock.sync's first {} callback?

I'm wondering if we can cover this gap by using Clock.now right after Clock.sync:

func synchronize(with pool: ...) {
   Clock.sync(
      // ...
      first: { publisher.publishAsync(...) } // first approximation
      completion: { publisher.publishAsync(...) } // stable result
   )

   publisher.publishAsync(Clock.now - currentDeviceTime()) // ← this
}

Kronos seems to integrate with UserDefaults and stores the last-known NTP time in there. With this change we should read it back as soon as possible. This will be thread safe and will be as precise as before, no?

[ncreated]

Is it possible to get now but not offset, or this is just a sanity fallback? In any case, it would be good to have inline comment on this, otherwise it seems unclear.

[maxep]

Yes, my comment above is misplaced. This is indeed a fallback in case the last sample returns a nil offset.
Th now parameter is Clock.now, so it is possible to have now but not offset.

[maxep]

I've updated my comment, LMKWYT!

[maxep]

Yes, Kronos first loads the previous offset from the UserDefaults. Their interface is not public, but we can indeed do this workaround 👍 It is thread-safe since they load from the user defaults on the current thread, but they could change that behavior any time.

[ncreated]

Yes, Kronos first loads the previous offset from the UserDefaults. Their interface is not public, but we can indeed do this workaround 👍 It is thread-safe since they load from the user defaults on the current thread, but they could change that behavior any time.

It's IMO fine 👍. Kronos now requires using Clock.sync and Clock.now on the same thread (which we do now 🚀), and it's hard to to imagine this changing to "must be used on different threads".

[maxep]

Kronos now requires using Clock.sync and Clock.now on the same thread

Actually, their requirement should be to 'invoke Clock.now on the main thread'. If, at some point, they decide to dispatch loadFromDefaults on the main thread, then we would still risk a data race.