search

pion / ice

A Go implementation of ICE
https://pion.ly/
MIT License
424 stars 158 forks source link

Doubts about discarding monotonic time #697

Closed jech closed 3 months ago

jech commented 3 months ago

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 e1c2d85530f0f9e96935c3598cc68b6c596c1a39, 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).

paulwe commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.

jech commented 3 months ago

Thanks!