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

Use init() for DefaultEntropy() instead of sync.Once #122

Open
ivanjaros opened this issue Jan 17, 2025 · 5 comments
Open

Use init() for DefaultEntropy() instead of sync.Once #122

ivanjaros opened this issue Jan 17, 2025 · 5 comments

Comments

@ivanjaros
Copy link

sync.Once uses atomic load for each call. This makes little sense when entropy should be established once via init().

@peterbourgon
Copy link
Member

peterbourgon commented Jan 17, 2025

Where do you see sync.Once? defaultEntropy is defined here.

@ivanjaros
Copy link
Author

Image

@ivanjaros
Copy link
Author

in the master https://github.com/oklog/ulid/blob/main/ulid.go#L132 you are passing function definition to where you expect io.Reader, so you need to pass defaultEntropy() instead of defaultEntropy BUT that also means you are passing new entropy/seed with each call to must news instead of having a global one. so this code makes little sense, compared to tagged 2.1.0.

Image

@ivanjaros
Copy link
Author

ivanjaros commented Jan 22, 2025

actually, i overlooked line 138. so now it looks better, though now it looks like the entropy source is generated during compilation time and is fixed, which is never good.

@peterbourgon
Copy link
Member

defaultEntropy is just that, the default source of entropy for the package. It's constructed and assigned by the in-line function defined and evaluated between lines 135 and 138 -- which is just an alternative way of computing and assigning a value to a package global variable, versus doing the same work in a func init(), and I think the current approach is more precise and generally better.

If you don't want to use the package global default entropy source, you don't have to. You just define whatever entropy source you want, and then pass that entropy source to (different) ULID constructors, to construct your ULIDs. This is fine.

We can probably make a new release that reflects the current main, which lags the most recent release.

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

No branches or pull requests

2 participants