search

go-ping / ping

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

Protect stats with RWMutex #151

Closed jraby closed 3 years ago

jraby commented 3 years ago

This is more of a request for comments than a proper PR (and it is branched off #150 so only the last commit should be reviewed )

A RWMutex is added to protect access to the Pinger's internal statistics. Updates to rtts is moved to updateStats so they happen under the lock.

There's (at least) one issue however: PacketsSent and PacketsRecvDuplicates can still be updated while Statistics() is running, and thus could yield strange results (sent<recv)

To fix this, atomics could be used, but it would require changing the types of the fields from int to int64, thus changing the public api of Pinger. (One could argue that those fields should probably not be exported to begin with, but that would break the api too)

Any thoughts?

SuperQ commented 3 years ago

Yessssssssssssssssssss.

jraby commented 3 years ago

Revisiting this, I think it could be possible to add a unexported uint64 (or int64) for packetsSent and duplicatesrecv. Updatng them using atomic.AddUint64 and accessing them in the stats with atomic.LoadUint64.

The exported fields can stay there and be updated as they are right now, but they wouldn't be used in Statistics(). With that, I think Statistics would be safe, or at least safer.

jraby commented 3 years ago

Rebased