go-ping / ping

ICMP Ping library for Go
MIT License
1.33k stars 345 forks source link

return value of time.Time.UnixNano() is not a monotonic timestamp #163

Closed tangxinfa closed 1 year ago

tangxinfa commented 3 years ago

timeToBytes() call UnixNano() on time.Time instance for get a integer timestamp, and serialized into binary, then send it to remote server, when response received, it convert it to time.Time instance for calculate rtt.

When the system time(wall time) changed in the middle, such as by ntp time synchronization, the calculated result may incorrect.

May be we can get a monotonic timestamp like bellow:

var monoStart time.Time

func init() {
    monoStart = time.Now()
}
func MonoUnixNano(t time.Time) time.Duration {
    return t.Sub(monoStart)
}
CHTJonas commented 3 years ago

Good spot! As I see it there are a few ways to fix this:

  1. Import a third-party timing library that works in monotonic clocks.
  2. Measure all time relatively, as you suggest.
  3. Remove the need to encode times into ICMP packets.

Regarding number 3, the ICMP RFCs (RFC 792 for IPv4 and RFC 4443 for IPv6) make clear that you don't have to encode timestamps in your ICMP packets as we currently do. I think we may be better off storing the time a packet is emitted locally and matching against the packet's tracker when it returns, then doing a monotonic t.Sub to get the delta.