labstack / gommon

Common packages for Go
MIT License
536 stars 100 forks source link

feat(random): use crypto/rand for random string generator #55

Closed ichigozero closed 10 months ago

ichigozero commented 1 year ago

The current implementation for random package relied on globally seeded value. If any other part of the programs call rand.Seed, the string generate by random package will also be affected.

To fix this issue, instead of calling rand.Seed during initialization, NewSource is called and the return value is stored inside Random struct.

I made changes by referring to the source code from the link below. https://github.com/labstack/echo/blob/e6b96f8873fed46e71e0d34cddb81c533167f954/middleware/util.go#L69

I also did some benchmarks before and after the update.

Before

goos: linux
goarch: amd64
pkg: github.com/labstack/gommon/random
cpu: Intel(R) Core(TM) i7-4702MQ CPU @ 2.20GHz
BenchmarkRandomString-8      1472410           806.7 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1478504           812.3 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1484530           806.4 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1470031           801.5 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1492831           809.4 ns/op        64 B/op          2 allocs/op

After (with sync.Mutex)

goos: linux
goarch: amd64
pkg: github.com/labstack/gommon/random
cpu: Intel(R) Core(TM) i7-4702MQ CPU @ 2.20GHz
BenchmarkRandomString-8      1507052           771.5 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1549957           775.3 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1519629           774.4 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1540794           774.6 ns/op        64 B/op          2 allocs/op
BenchmarkRandomString-8      1548172           777.5 ns/op        64 B/op          2 allocs/op

After (with sync.Pool)

goos: linux
goarch: amd64
pkg: github.com/labstack/gommon/random
cpu: Intel(R) Core(TM) i7-4702MQ CPU @ 2.20GHz
BenchmarkRandomString-8      2655366           446.7 ns/op       112 B/op          3 allocs/op
BenchmarkRandomString-8      2629251           451.6 ns/op       112 B/op          3 allocs/op
BenchmarkRandomString-8      2613632           456.5 ns/op       112 B/op          3 allocs/op
BenchmarkRandomString-8      2638729           458.5 ns/op       112 B/op          3 allocs/op
BenchmarkRandomString-8      2577884           462.6 ns/op       112 B/op          3 allocs/op
aldas commented 1 year ago

maybe https://github.com/labstack/echo/blob/e6b96f8873fed46e71e0d34cddb81c533167f954/middleware/util.go#L69 would be suitable here also.

ichigozero commented 1 year ago

maybe https://github.com/labstack/echo/blob/e6b96f8873fed46e71e0d34cddb81c533167f954/middleware/util.go#L69 would be suitable here also.

Thank you for the comment. I will take a look the link you mentioned and update the PR accordingly.

ichigozero commented 1 year ago

@aldas I have updated the source code as you suggested. Please let me know what you think.

ichigozero commented 11 months ago

@aldas Hi, just wondering what is the progress for this PR?