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.

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

Doubts about discarding monotonic time #697

Closed
jech opened this issue May 9, 2024 · 4 comments · Fixed by #699
Closed

Doubts about discarding monotonic time #697

jech opened this issue May 9, 2024 · 4 comments · Fixed by #699

Comments

@jech
Copy link
Contributor

jech commented May 9, 2024

Hi,

I'm looking at @paulwe optimisations to pion/ice right now, thanks a lot for the work, Paul.

I have a doubt about commit e1c2d85, where the lastSent and lastReceived fields of candidateBase have been changed from time.Time to Unix time. In doing so, they no longer contain monotonic time, and therefore become susceptible to stepping clocks.

Paul, can you confirm that this change will not cause trouble if the system clock is stepped? If not, then I suggest that either the commit should be reverted, or we should use a technique similar to what I do in https://github.com/jech/storrent/blob/master/mono/mono.go, where I had exactly the same problem (albeit with just second granularity).

jech added a commit to jech/ice that referenced this issue May 9, 2024
This reverts commit edb6929.

In that commit, active time was changed from time.Time to
Unix time in order to avoid allocations. Unfortunately, that
has the side effect of discarding the monotonic component of
time.Time, and therefore makes our code vulnerable to stepping
of the system clock.

Fixes pion#697
@paulwe
Copy link
Contributor

paulwe commented May 25, 2024

in practice i don't think it matters. on any well behaved system the aberrations in wall clock time should be smaller than the tolerance for timeout/keepalive scheduling.

because it's part of the public api we can't really do anything more clever. go ahead and revert it if you like. none of this had a measurable impact on my target metric.

@jech
Copy link
Contributor Author

jech commented May 26, 2024

in practice i don't think it matters. on any well behaved system the aberrations in wall clock time should be smaller than the tolerance for timeout/keepalive scheduling.

What happens if the system clock is stepped?

Some systems have no battery-backed clock. On such systems, the system clock is stepped after NTP has had time to converge, typically a couple of minutes after boot. If I understand your patch correctly, it will cause Pion to break after the clock is stepped.

none of this had a measurable impact on my target metric.ou

Did you test while stepping the clock?

go ahead and revert it if you like

@Sean-Der could you please consider PRs #698 and #699? They unbreak Pion on systems with no battery-backed clock (such as Raspberry Pis and other SBCs).

@paulwe
Copy link
Contributor

paulwe commented May 26, 2024

sorry i thought you had commit or i would have done it myself. like i said, this didn't fix the issue i was trying to solve so i'm not super attached to it.

paulwe pushed a commit that referenced this issue May 26, 2024
This reverts commit edb6929.

In that commit, active time was changed from time.Time to
Unix time in order to avoid allocations. Unfortunately, that
has the side effect of discarding the monotonic component of
time.Time, and therefore makes our code vulnerable to stepping
of the system clock.

Fixes #697
@jech
Copy link
Contributor Author

jech commented May 26, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants