Refactor TrueTime class to improve synchronization logic

[enjikaka]

Long over-night or over-weekend sessions could get wrong timestamps.

This pull request includes changes to the TrueTime class in the packages/true-time/src/index.ts file. The changes are focused on modifying the #synced private member and its usage in the synchronize method to ensure the synchronization process doesn't occur more frequently than once every 1,000,000 milliseconds.

• packages/true-time/src/index.ts: The #synced private member was modified from a boolean to a number or undefined. This change allows the storage of the timestamp of the last successful synchronization.
• packages/true-time/src/index.ts: In the synchronize method, a check was added to ensure that synchronization doesn't occur if the last successful synchronization was less than 1,000,000 milliseconds ago.
• packages/true-time/src/index.ts: The #synced private member is now set to the current timestamp (Date.now()) after a successful synchronization.

[osmestad]

I have been thinking there was a problem with this TrueTime implementation, but I was not able to set up a test to prove it, glad others found it :-)

For the solution, I don't understand how this can work. I think the problem is that we use (high resolution) timers, and not Date.now().

The problem being that the high resolution timer might be paused when the computer is in sleep, so the calculation: initialServerTime + (currentClientTime - initialClientTime) = will give a (server adjusted) time that is in the past if the clock was paused.

This PR updates initialServerTime without changing initialClientTime?
I think switching to Date.now() will be simpler to get correct, even if the resolution might be a bit lower.

Clocks and timers are a bit of a deep-dive, but this has some good explanations and links: w3c/hr-time#115
It links several Chromium bugs I think are relevant:
https://issues.chromium.org/issues/41450546
https://issues.chromium.org/issues/40765101
https://issues.chromium.org/issues/40296804

[enjikaka]

I have been thinking there was a problem with this TrueTime implementation, but I was not able to set up a test to prove it, glad others found it :-)

For the solution, I don't understand how this can work. I think the problem is that we use (high resolution) timers, and not Date.now().

The problem being that the high resolution timer might be paused when the computer is in sleep, so the calculation: initialServerTime + (currentClientTime - initialClientTime) = will give a (server adjusted) time that is in the past if the clock was paused.

This PR updates initialServerTime without changing initialClientTime? I think switching to Date.now() will be simpler to get correct, even if the resolution might be a bit lower.

Clocks and timers are a bit of a deep-dive, but this has some good explanations and links: w3c/hr-time#115 It links several Chromium bugs I think are relevant: https://issues.chromium.org/issues/41450546 https://issues.chromium.org/issues/40765101 https://issues.chromium.org/issues/40296804

Then we should be fine if we also sync clientStartTime along with the #synced change. We don't use the high res time stamp as an absolute value, just a measure of time elapsed and between two set marks and then adjust the time based on the server time. Using performance.timeOrigin instead of #clientStartTime would maybe also be good?

[osmestad]

Then we should be fine if we also sync clientStartTime along with the #synced change. We don't use the high res time stamp as an absolute value, just a measure of time elapsed and between two set marks and then adjust the time based on the server time. Using performance.timeOrigin instead of #clientStartTime would maybe also be good?

I think the problem is we can't detect if the performance time clock has been paused or not? (and so the duration between start and stop is possibly lower than it should be)

[enjikaka]

I think the problem is we can't detect if the performance time clock has been paused or not? (and so the duration between start and stop is possibly lower than it should be)

Guess we could store and compare a drift value of performance.now vs date.now to detect a pause? 🤔

[osmestad]

If this should be allowed to run more than once, I can't see how it can work if not also setting clientStartTime?

[osmestad]

This "feels" like it is getting complicated, are we sure there are any benefits to not just replacing performance.now() with Date.now() and keeping this module simple?

[enjikaka]

Then we'd have to re-implement performance.mark() with Date.now too... :/

[osmestad]

Yeah it is a bit unfortunate that that is mixed in here, makes this more complex than it should be :-(

Might need to think a bit how we can avoid that..

[osmestad]

I solved it in the other PR by setting startTime in each performance.mark() call, we can also consider making a trueTime.mark() that abstracts that maybe?

[TheHaff]

Suggestion (possibly dumb): race calls to different time providing servers like google, apple and idk? for the initial timestamp. Might yield some minor improvement based on location and or downtimes? Iterative in a next pr ofc

[osmestad]

I added an alternative solution: #66 I think that is simpler and less fragile?

[enjikaka]

I added an alternative solution: #66 I think that is simpler and less fragile?

Looks more sane, but with that we still have 2 issues that this PR doesn't:

• client clock change doesn't resync with server after first init (Date.now() can change if os time is changed after app is stared)
• measures over computer sleep will be incorrect since Date.now() isn't frozen, so a measure between two marks could be 2-3 days instead of 0, increasing for example the measure of start->end timestamp. Don't know if that is too cornery case to care or not... but if we're betting hard on playlog that needs to be right?

[osmestad]

I added an alternative solution: #66 I think that is simpler and less fragile?

Looks more sane, but with that we still have 2 issues that this PR doesn't:

• client clock change doesn't resync with server after first init (Date.now() can change if os time is changed after app is stared)

Yes can try to bring that over from this PR! (not sure how commonly it happens, but should not hurt to be on the safe side :-) )

• measures over computer sleep will be incorrect since Date.now() isn't frozen, so a measure between two marks could be 2-3 days instead of 0, increasing for example the measure of start->end timestamp. Don't know if that is too cornery case to care or not... but if we're betting hard on playlog that needs to be right?

I don't think I understand the expectation here? (performance.now() is not guaranteed to be frozen, as previously linked, that is an implementation bug on MacOS (and Linux), in Windows it should tick during sleep?)

[osmestad]

• client clock change doesn't resync with server after first init (Date.now() can change if os time is changed after app is stared)

Yes can try to bring that over from this PR! (not sure how commonly it happens, but should not hurt to be on the safe side :-) )

Ported: 899eb92 (could now also remove the synced flag!)

[enjikaka]

I don't think I understand the expectation here? (performance.now() is not guaranteed to be frozen, as previously linked, that is an implementation bug on MacOS (and Linux), in Windows it should tick during sleep?)

Ah okey, if it's not guaranteed to be frozen then never mind. Guess those cases could be detected by back-end and adjusted/filtered properly anyway. I'll close this and we'll put our bets on your PR :)