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

lastSent/keepAlive logic should not use wall clock #300

Closed
johnhydroware opened this issue Apr 4, 2019 · 4 comments · Fixed by #305
Closed

lastSent/keepAlive logic should not use wall clock #300

johnhydroware opened this issue Apr 4, 2019 · 4 comments · Fixed by #305

Comments

@johnhydroware
Copy link
Contributor

I happened to look through the source code and saw that lastSent/lastReceived uses epoch time. But the logic for keepAlive should really use time elapsed. Therefore I suggest that
lastSent int64 and lastReceived int64 uses time.Time instead. Then when comparing times the monotonic clock will be used instead.

With the current logic the keep alive will break if the wall clock on the computer is changed.

Please advice if I have missunderstood something or if someone have a better solution.

@alsm
Copy link
Contributor

alsm commented Apr 4, 2019

I believe you are correct that this could be an issue, the problem is that those values are accessed from different goroutines so use the sync/atomic package for accessing and setting their values. It would need a new struct wrapping a time.Time and a mutex to give the same function. Don't suppose you fancy having a go at doing this?

@johnhydroware
Copy link
Contributor Author

I can see if I get some time beginning of next week. I'm not really sure about the need for struct and mutex though. The code right now uses StoreInt64/LoadInt64.
So I should just need to define it as lastReceived atomic.Value
and then use c.lastReceived.Store(time.Now()) and c.lastReceived.Load().(time.Time) since the object stored is never mutated; when time is changed a new time object is created.

@alsm
Copy link
Contributor

alsm commented Apr 4, 2019

ok cool, hadn't used atomic.Value before, I'll look at doing that

@johnhydroware
Copy link
Contributor Author

johnhydroware commented Apr 15, 2019

I have tried to make a pull request for this, put I can't seem to get the license thing to work. But I have a "Eclipse Contributor Agreement" green checkbox on my profile. I only get "This validation request is closed or unavailable." when clicking "details".

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