Add thread safety to default random (#16174)

`Random::DEFAULT` is not an alias for the `Random::PCG32` default implementation but a global instance of it. It's initialized once then shared across all threads, but the algorithm itself is not thread safe (two or more threads generating a random number will mess with the internal state).

Co-authored-by: Johannes Müller <straightshoota@gmail.com>

Author: ysbaddaden

Makefile

@@ -46,7 +46,7 @@ override FLAGS += -D strict_multi_assign -D preview_overload_order $(if $(releas
 # NOTE: USE_PCRE1 is only used for testing compatibility with legacy environments that don't provide libpcre2.
 # Newly built compilers should never be distributed with libpcre to ensure syntax consistency.
 override COMPILER_FLAGS += $(if $(interpreter),,-Dwithout_interpreter )$(if $(docs_sanitizer),,-Dwithout_libxml2 ) -Dwithout_openssl -Dwithout_zlib $(if $(USE_PCRE1),-Duse_pcre,-Duse_pcre2)
-SPEC_WARNINGS_OFF := --exclude-warnings spec/std --exclude-warnings spec/compiler --exclude-warnings spec/primitives --exclude-warnings src/float/printer
+SPEC_WARNINGS_OFF := --exclude-warnings spec/std --exclude-warnings spec/compiler --exclude-warnings spec/primitives --exclude-warnings src/float/printer --exclude-warnings src/random.cr
 override SPEC_FLAGS += $(if $(verbose),-v )$(if $(junit_output),--junit_output $(junit_output) )$(if $(order),--order=$(order) )
 CRYSTAL_CONFIG_LIBRARY_PATH := '$$ORIGIN/../lib/crystal'
 CRYSTAL_CONFIG_BUILD_COMMIT ?= $(shell git rev-parse --short HEAD 2> /dev/null)

spec/std/spec/context_spec.cr

@@ -6,7 +6,7 @@ describe Spec::ExampleGroup do
       root = build_spec("f.cr", count: 20)
 
       before_randomize = all_spec_descriptions(root)
-      root.randomize(Random::DEFAULT)
+      root.randomize(Random.new)
       after_randomize = all_spec_descriptions(root)
 
       after_randomize.should_not eq before_randomize

src/enumerable.cr

@@ -1596,8 +1596,9 @@ module Enumerable(T)
     ary = Array(T).new(n)
     return ary if n == 0
 
-    # FIXME: thread unsafe (#each may yield and the fiber switch threads)
-    rng = random || Random::DEFAULT
+    # must split the default random instance (thread local) because #each might
+    # yield the current fiber that may be resumed by another thread
+    rng = random || Random.split_on_stack
 
     each_with_index do |elem, i|
       if i < n
@@ -1636,8 +1637,9 @@ module Enumerable(T)
     value = uninitialized T
     found = false
 
-    # FIXME: thread unsafe (#each may yield and the fiber switch threads)
-    rng = random || Random::DEFAULT
+    # must split the default random instance (thread local) because #each might
+    # yield the current fiber that may be resumed by another thread
+    rng = random || Random.split_on_stack
 
     each_with_index do |elem, i|
       if !found

src/indexable.cr

@@ -965,7 +965,7 @@ module Indexable(T)
   # ```
   def sample(random : Random? = nil)
     raise IndexError.new("Can't sample empty collection") if size == 0
-    rng = random || Random::DEFAULT
+    rng = random || Random.thread_default
     unsafe_fetch(rng.rand(size))
   end
 

src/indexable/mutable.cr

@@ -271,7 +271,7 @@ module Indexable::Mutable(T)
   # a                          # => [3, 2, 4, 5, 1]
   # ```
   def shuffle!(random : Random? = nil) : self
-    rng = random || Random::DEFAULT
+    rng = random || Random.thread_default
     (size - 1).downto(1) do |i|
       j = rng.rand(i + 1)
       swap(i, j)

src/kernel.cr

@@ -593,6 +593,7 @@ end
 
         # additional reinitialization
         ->Random::DEFAULT.new_seed,
+        -> { Random.thread_default.new_seed },
       ] of -> Nil
     end
   end

src/pointer.cr

@@ -389,7 +389,7 @@ struct Pointer(T)
   # ptr.to_slice(4) # => Slice[3, 4, 1, 2]
   # ```
   def shuffle!(count : Int, random : Random? = nil)
-    rng = random || Random::DEFAULT
+    rng = random || Random.thread_default
     (count - 1).downto(1) do |i|
       j = rng.rand(i + 1)
       swap(i, j)

src/random.cr

@@ -50,8 +50,30 @@ require "random/pcg32"
 # slower but cryptographically secure, so a third party can't deduce incoming
 # numbers.
 module Random
+  @[Deprecated("Use `#rand`, `Random.next_int` or `Random::Secure.random_bytes` for example, or create a local instance with `Random.new` instead.")]
   DEFAULT = PCG32.new
 
+  # :nodoc:
+  thread_local(thread_default : ::Random) do
+    ::Random::PCG32.new.as(::Random)
+  end
+
+  # :nodoc:
+  #
+  # Same as `Random.thread_default#split` but allocates the dup on the stack
+  # when possible (hence the macro).
+  macro split_on_stack
+    {% if compare_versions(Crystal::VERSION, "1.12.0") >= 0 %}
+      %thread_rng = ::Random.thread_default.as(::Random::PCG32)
+      %buf = uninitialized ::ReferenceStorage(::Random::PCG32)
+      %copy = ::Random::PCG32.unsafe_construct(pointerof(%buf), %thread_rng)
+      %thread_rng.split_internal(%copy)
+      %copy
+    {% else %}
+      ::Random.thread_default.split
+    {% end %}
+  end
+
   # Initializes an instance with the given *seed* and *sequence*.
   def self.new(seed, sequence = 0_u64)
     PCG32.new(seed.to_u64, sequence)
@@ -62,6 +84,11 @@ module Random
     PCG32.new
   end
 
+  # Reseed the generator.
+  def new_seed
+    raise NotImplementedError.new("{{@type}}#new_seed")
+  end
+
   # Generates a random unsigned integer.
   #
   # The integers must be uniformly distributed between `0` and
@@ -456,22 +483,22 @@ module Random
 
   # See `#next_bool`.
   def self.next_bool : Bool
-    DEFAULT.next_bool
+    thread_default.next_bool
   end
 
   # See `#next_int`.
   def self.next_int : Int32
-    DEFAULT.next_int
+    thread_default.next_int
   end
 
   # See `#rand`.
   def self.rand : Float64
-    DEFAULT.rand
+    thread_default.rand
   end
 
   # See `#rand(x)`.
   def self.rand(x)
-    DEFAULT.rand(x)
+    thread_default.rand(x)
   end
 end
 

src/range.cr

@@ -345,7 +345,7 @@ struct Range(B, E)
   #
   # Raises `ArgumentError` if `self` is an open range.
   def sample(random : Random? = nil)
-    rng = random || Random::DEFAULT
+    rng = random || Random.thread_default
 
     {% if B < Int && E < Int %}
       rng.rand(self)
@@ -371,7 +371,7 @@ struct Range(B, E)
   # once. Thus, *random* will be left in a different state compared to the
   # implementation in `Enumerable`.
   def sample(n : Int, random : Random? = nil)
-    rng = random || Random::DEFAULT
+    rng = random || Random.thread_default
 
     if self.begin.nil? || self.end.nil?
       raise ArgumentError.new("Can't sample an open range")