Skip to content

Setting the random seed every time is not a good idea #10

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

Merged
merged 2 commits into from
Mar 12, 2025

Conversation

ufownl
Copy link
Contributor

@ufownl ufownl commented Mar 10, 2025

Remove math.randomseed call from totp_new_key function.

@leslie-tsang
Copy link
Owner

Yes, would be better to init math.randomseed at the beginning of the module instead of function call. :)

@ufownl
Copy link
Contributor Author

ufownl commented Mar 10, 2025

Calling it at the beginning of the module may change the user's fixed seed, so it might be a better choice to let users call it themselves.

@leslie-tsang
Copy link
Owner

so it might be a better choice to let users call it themselves.

I'm afraid that's not a good way to handle this, which require the user to be familiar with the library's implementation. :)

@ufownl
Copy link
Contributor Author

ufownl commented Mar 10, 2025

Indeed, a better approach is to use a separate random generator in each instance so that it can independently set a fixed seed or a random one. However, Lua's built-in random generator is global, and implicitly setting the random seed can cause many problems.

@leslie-tsang
Copy link
Owner

setting the random seed can cause many problems.

That's a bit of an overstatement in the real world. you should never expect a exact result of RNG, even a PRNG. :)

It shouldn't cause any issue as the random number seeds are not duplicated in such a scenario.

@ufownl
Copy link
Contributor Author

ufownl commented Mar 10, 2025

  1. In some cases, when we need reproducible results, the fixed random seed is required. This is very common in machine learning or debugging, etc.
  2. In OpenResty, users may load modules during the initialization phase of the master process, and then the same seed that is set implicitly will be forked to the worker processes, so they still need to re-set the seed in each worker process.

It should be common sense to set the seed before all random operations. Setting it implicitly when loading the module does not simplify usage and introduces global side effects. I don't think this is a good way.

@leslie-tsang
Copy link
Owner

Sounds fair, let's move it to a separate function and add test cases.
Would you like to take over? :)

@ufownl
Copy link
Contributor Author

ufownl commented Mar 10, 2025

Ok, I should be able to do it tomorrow. :)

@ufownl
Copy link
Contributor Author

ufownl commented Mar 12, 2025

unit-test (ubuntu-18.04, *) actions seem to have been queued for a long time, maybe they need manual intervention.

@leslie-tsang
Copy link
Owner

unit-test (ubuntu-18.04, *) actions seem to have been queued for a long time, maybe they need manual intervention.

Yep, I will fix it. :)

Copy link
Owner

@leslie-tsang leslie-tsang left a comment

Choose a reason for hiding this comment

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

LGTM

@leslie-tsang leslie-tsang merged commit d5d856a into leslie-tsang:master Mar 12, 2025
2 of 4 checks passed
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.

2 participants