Skip to content

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

Merged
aldas merged 10 commits into
labstack:masterfrom
dreamkyast0:random
Nov 9, 2023
Merged

feat(random): use crypto/rand for random string generator#55
aldas merged 10 commits into
labstack:masterfrom
dreamkyast0:random

Conversation

@dreamkyast0

@dreamkyast0 dreamkyast0 commented Aug 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

aldas commented Aug 12, 2023

Copy link
Copy Markdown
Contributor

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

@dreamkyast0

dreamkyast0 commented Aug 12, 2023

Copy link
Copy Markdown
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.

@dreamkyast0

dreamkyast0 commented Aug 12, 2023

Copy link
Copy Markdown
Contributor Author

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

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

dreamkyast0 commented Oct 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

aldas
aldas approved these changes Nov 9, 2023
View reviewed changes

@aldas aldas left a comment

Copy link
Copy Markdown
Contributor

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
@dreamkyast0 dreamkyast0 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

Reviewers

@aldas aldas aldas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dreamkyast0 @aldas