Thread safety
SasLuca opened this issue · 0 comments
Problem
Right now the library uses a global instance of RandomNumberGenerator
, however RandomNumberGenerator
is not thread safe. The library does not properly document this aspect and given the intended common usage of nanoids is to generate random ids in web servers, this might be a silent issue that affects developers.
In the original JS implementation this is not an issue due to JS not being multi threaded but C# is.
Potential solution
We could address this by making CryptoRandom._r
a member field rather than a static field, then making the global CryptoRandom
instance ThreadStatic
(or as it is known in other languages threadlocal
). This would ensure there is one instance separate per thread thus avoiding any concurrency issues without the need for locks, keeping the id generation fast as advertised.
There also exists Random.Shared
in .net6
and above but it is not cryptographically secure and it wouldn't cover all the targeted .net versions of the library.
Here is a great article on the topic: https://andrewlock.net/building-a-thread-safe-random-implementation-for-dotnet-framework/
Breaking change considerations
This is technically a breaking change in theory, in case anyone abused these internal details, but arguably no one should have been doing that. It is more likely if anything that people currently using the library having thread safety issues without realizing and we never know when those could cause issues for someone. Therefore I don't think such a change would be problematic from this perspective, if anything it is greatly needed.