-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
inwx: improve sleep calculation #2086
Conversation
@gnoack can you take a look? |
2a55dd3
to
9f1e3b4
Compare
if err != nil { | ||
return err | ||
} | ||
|
||
d.previousUnlock = time.Now() | ||
d.previousUnlock = now.Truncate(30 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time.Truncate() [1] is rounding down to multiples of the duration since the Go zero time [2], but that is a different baseline time than the Unix epoch on 1970-01-01. (I assume there are probably various leap seconds or other time drift corrections in between, so that these two points in time are not aligned on the same 30-second period?)
[1] https://pkg.go.dev/time#Time.Truncate
[2] https://pkg.go.dev/time#Time.IsZero
Apart from that, I think this approach works well -- if you replace this Truncate call with one which truncates to multiples of 30 seconds since the Unix epoch, (or if you double check that I am wrong about the leap seconds theory), that should work. (maybe it's worth calculating a valid boundary by hand for the test and exercising it there, to be sure)
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to see what can be difference as we are using second 🤔
https://go.dev/play/p/SECZB_iROSw
Maybe I miss something, can you give me more explanation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on:
leap seconds doesn't seem to be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, and your solution is more elegant indeed. Let's use Time.Truncate()
:)
package trunctime
import (
"testing"
"time"
)
func FuzzTruncation(f *testing.F) {
for _, uts := range []int64{
0, 1234567890, 99,
} {
f.Add(uts)
}
f.Fuzz(func(t *testing.T, uts int64) {
if uts <= 0 {
t.Skip("Negative timestamp")
}
ts := time.Unix(uts, 0)
tstrunc := ts.Truncate(30 * time.Second)
utstrunc := time.Unix(30*(uts/30), 0)
if !tstrunc.Equal(utstrunc) {
t.Errorf("tstrunc %v != utstrunc %v", tstrunc, utstrunc)
}
})
}
I threw a fuzzer at it, to be sure, and it did not find any differences after a few tens-of-million attempts :)
...
fuzz: elapsed: 3m36s, execs: 49078748 (227838/sec), new interesting: 2 (total: 5)
fuzz: elapsed: 3m39s, execs: 49767357 (229712/sec), new interesting: 2 (total: 5)
fuzz: elapsed: 3m42s, execs: 50445798 (226109/sec), new interesting: 2 (total: 5)
^Cfuzz: elapsed: 3m43s, execs: 50697296 (229938/sec), new interesting: 2 (total: 5)
PASS
ok github.com/gnoack/go-experiment/trunctime 223.097s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this looks good!
#2084 (comment)