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

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

Merged
merged 10 commits into from
Nov 9, 2023

Conversation

ichigozero
Copy link
Contributor

@ichigozero ichigozero commented Aug 12, 2023

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
Copy link
Contributor

aldas commented Aug 12, 2023

@ichigozero
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@ichigozero ichigozero changed the title feat(random): use local random seed instead global one feat(random): use crypto/rand for random string generator Aug 14, 2023
@ichigozero
Copy link
Contributor Author

ichigozero commented Oct 5, 2023

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

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aldas aldas merged commit 508eabf into labstack:master Nov 9, 2023
@ichigozero ichigozero deleted the random branch November 10, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants