Add thread safety to default random

Makefile

@@ -42,7 +42,7 @@ SOURCES := $(shell find src -name '*.cr')
 SPEC_SOURCES := $(shell find spec -name '*.cr')
 MAN1PAGES := $(patsubst doc/man/%.adoc,man/%.1,$(wildcard doc/man/*.adoc))
 override FLAGS += -D strict_multi_assign -D preview_overload_order $(if $(release),--release )$(if $(stats),--stats )$(if $(progress),--progress )$(if $(threads),--threads $(threads) )$(if $(debug),-d )$(if $(static),--static )$(if $(LDFLAGS),--link-flags="$(LDFLAGS)" )$(if $(target),--cross-compile --target $(target) )$(if $(interpreter),,-Dwithout_interpreter )
-SPEC_WARNINGS_OFF := --exclude-warnings spec/std --exclude-warnings spec/compiler --exclude-warnings spec/primitives
+SPEC_WARNINGS_OFF := --exclude-warnings spec/std --exclude-warnings spec/compiler --exclude-warnings spec/primitives --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)

samples/sdl/tv.cr

@@ -64,7 +64,7 @@ class ColorMaker
   end
 
   def make_alpha_color(multiplier)
-    rand = Random::DEFAULT.next_int
+    rand = Random.next_int
     r = ((rand >> 16) % 256).to_i
     g = ((rand >> 8) % 256).to_i
     b = (rand % 256).to_i

spec/std/file_spec.cr

@@ -64,7 +64,7 @@ describe "File" do
 
       rbuf = Bytes.new(5120)
       wbuf = Bytes.new(5120)
-      Random::DEFAULT.random_bytes(wbuf)
+      Random::Secure.random_bytes(wbuf)
 
       File.open(path, "r") do |reader|
         # opened fifo for read: wait for thread to open for write

spec/std/random_spec.cr

@@ -196,7 +196,11 @@ describe "Random" do
   end
 
   it "gets a random bool" do
-    Random::DEFAULT.next_bool.should be_a(Bool)
+    Random.next_bool.should be_a(Bool)
+  end
+
+  it "gets a random int" do
+    Random.next_int.should be_a(Int32)
   end
 
   it "generates by accumulation" do

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.default)
       after_randomize = all_spec_descriptions(root)
 
       after_randomize.should_not eq before_randomize

src/array.cr

@@ -1604,9 +1604,10 @@ class Array(T)
     shift { nil }
   end
 
-  # Returns an array with all the elements in the collection randomized
-  # using the given *random* number generator.
-  def shuffle(random : Random = Random::DEFAULT) : Array(T)
+  # Returns a new instance with all elements in the collection randomized.
+  #
+  # See `Indexable::Mutable#shuffle!` for details.
+  def shuffle(random : Random? = nil) : Array(T)
     dup.shuffle!(random)
   end
 

src/colorize.cr

@@ -91,7 +91,7 @@
 # ```
 # require "colorize"
 #
-# "foo".colorize(Random::DEFAULT.next_bool ? :green : :default)
+# "foo".colorize(Random.next_bool ? :green : :default)
 # ```
 #
 # Available colors are:

src/crystal/system/file.cr

@@ -51,18 +51,23 @@ module Crystal::System::File
 
   LOWER_ALPHANUM = "0123456789abcdefghijklmnopqrstuvwxyz".to_slice
 
-  def self.mktemp(prefix : String?, suffix : String?, dir : String, random : ::Random = ::Random::DEFAULT) : {FileDescriptor::Handle, String, Bool}
+  def self.mktemp(prefix : String?, suffix : String?, dir : String, random : ::Random? = nil) : {FileDescriptor::Handle, String, Bool}
     flags = LibC::O_RDWR | LibC::O_CREAT | LibC::O_EXCL
     perm = ::File::Permissions.new(0o600)
 
     prefix = ::File.join(dir, prefix || "")
     bytesize = prefix.bytesize + 8 + (suffix.try(&.bytesize) || 0)
 
     100.times do
+      # must resolve the default random (thread local) on each iteration because
+      # `open` below may yield the current fiber and the fiber be resumed on
+      # another thread:
+      rng = random || ::Random.default
+
       path = String.build(bytesize) do |io|
         io << prefix
         8.times do
-          io.write_byte LOWER_ALPHANUM.sample(random)
+          io.write_byte LOWER_ALPHANUM.sample(rng)
         end
         io << suffix
       end

src/crystal/system/unix/process.cr

@@ -172,7 +172,7 @@ struct Crystal::System::Process
     end
   {% end %}
 
-  def self.fork(*, will_exec = false)
+  def self.fork(*, will_exec : Bool, &)
     newmask = uninitialized LibC::SigsetT
     oldmask = uninitialized LibC::SigsetT
 
@@ -191,18 +191,16 @@ struct Crystal::System::Process
     when 0
       # child:
       pid = nil
-      if will_exec
-        # notify event loop
-        Crystal::EventLoop.current.after_fork_before_exec
 
-        # reset signal handlers, then sigmask (inherited on exec):
-        Crystal::System::Signal.after_fork_before_exec
+      # after fork callback
+      yield
+
+      if will_exec
+        # reset sigmask (inherited on exec)
         LibC.sigemptyset(pointerof(newmask))
         LibC.pthread_sigmask(LibC::SIG_SETMASK, pointerof(newmask), nil)
       else
-        {% unless flag?(:preview_mt) %}
-          ::Process.after_fork_child_callbacks.each(&.call)
-        {% end %}
+        # restore sigmask
         LibC.pthread_sigmask(LibC::SIG_SETMASK, pointerof(oldmask), nil)
       end
     when -1
@@ -224,9 +222,10 @@ struct Crystal::System::Process
   def self.fork(&)
     {% raise("Process fork is unsupported with multithreaded mode") if flag?(:preview_mt) %}
 
-    if pid = fork
-      return pid
+    pid = fork(will_exec: false) do
+      ::Process.after_fork_child_callbacks.each(&.call)
     end
+    return pid if pid
 
     begin
       yield
@@ -243,7 +242,12 @@ struct Crystal::System::Process
   def self.spawn(command_args, env, clear_env, input, output, error, chdir)
     r, w = FileDescriptor.system_pipe
 
-    pid = self.fork(will_exec: true)
+    pid = self.fork(will_exec: true) do
+      # notify event loop and reset signal handlers
+      Crystal::EventLoop.current.after_fork_before_exec
+      Crystal::System::Signal.after_fork_before_exec
+    end
+
     if !pid
       LibC.close(r)
       begin

src/enumerable.cr

@@ -1565,19 +1565,27 @@ module Enumerable(T)
     reject { |e| pattern === e }
   end
 
-  # Returns an `Array` of *n* random elements from `self`, using the given
-  # *random* number generator. All elements have equal probability of being
-  # drawn. Sampling is done without replacement; if *n* is larger than the size
-  # of this collection, the returned `Array` has the same size as `self`.
+  # Returns an `Array` of *n* random elements from `self`. All elements have
+  # equal probability of being drawn. Sampling is done without replacement; if
+  # *n* is larger than the size of this collection, the returned `Array` has the
+  # same size as `self`.
   #
   # Raises `ArgumentError` if *n* is negative.
   #
   # ```
-  # [1, 2, 3, 4, 5].sample(2)                # => [3, 5]
-  # {1, 2, 3, 4, 5}.sample(2)                # => [3, 4]
-  # {1, 2, 3, 4, 5}.sample(2, Random.new(1)) # => [1, 5]
+  # [1, 2, 3, 4, 5].sample(2) # => [3, 5]
+  # {1, 2, 3, 4, 5}.sample(2) # => [3, 4]
   # ```
-  def sample(n : Int, random : Random = Random::DEFAULT) : Array(T)
+  #
+  # Uses the *random* instance when provided if the randomness needs to be
+  # controlled or to follow some traits. For example the following calls use a
+  # custom seed or a secure random source:
+  #
+  # ```
+  # {1, 2, 3, 4, 5}.sample(2, Random.new(1))  # => [1, 5]
+  # {1, 2, 3, 4, 5}.sample(2, Random::Secure) # => [2, 5]
+  # ```
+  def sample(n : Int, random : Random? = nil) : Array(T)
     raise ArgumentError.new("Can't sample negative number of elements") if n < 0
 
     # Unweighted reservoir sampling:
@@ -1588,40 +1596,56 @@ module Enumerable(T)
     ary = Array(T).new(n)
     return ary if n == 0
 
+    # 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.dup_and_split_default(stack: true)
+
     each_with_index do |elem, i|
       if i < n
         ary << elem
       else
-        j = random.rand(i + 1)
+        j = rng.rand(i + 1)
         if j < n
           ary.to_unsafe[j] = elem
         end
       end
     end
 
-    ary.shuffle!(random)
+    ary.shuffle!(rng)
   end
 
-  # Returns a random element from `self`, using the given *random* number
-  # generator. All elements have equal probability of being drawn.
+  # Returns a random element from `self`. All elements have equal probability of
+  # being drawn.
   #
   # Raises `IndexError` if `self` is empty.
   #
   # ```
   # a = [1, 2, 3]
-  # a.sample                # => 2
-  # a.sample                # => 1
-  # a.sample(Random.new(1)) # => 3
+  # a.sample # => 2
+  # a.sample # => 1
   # ```
-  def sample(random : Random = Random::DEFAULT) : T
+  #
+  # Uses the *random* instance when provided if the randomness needs to be
+  # controlled or to follow some traits. For example the following calls use a
+  # custom seed or a secure random source:
+  #
+  # ```
+  # a.sample(Random.new(1))  # => 3
+  # a.sample(Random::Secure) # => 1
+  # ```
+  def sample(random : Random? = nil) : T
     value = uninitialized T
     found = false
 
+    # 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.dup_and_split_default(stack: true)
+
     each_with_index do |elem, i|
       if !found
         value = elem
         found = true
-      elsif random.rand(i + 1) == 0
+      elsif rng.rand(i + 1) == 0
         value = elem
       end
     end

src/http/web_socket/protocol.cr

@@ -136,7 +136,7 @@ class HTTP::WebSocket::Protocol
   private def write_payload(data)
     return @io.write(data) unless @masked
 
-    key = Random::DEFAULT.next_int
+    key = Random.next_int
     mask_array = key.unsafe_as(StaticArray(UInt8, 4))
     @io.write mask_array.to_slice
 

src/indexable.cr

@@ -950,21 +950,31 @@ module Indexable(T)
   #
   # ```
   # a = [1, 2, 3]
-  # a.sample                # => 3
-  # a.sample                # => 1
+  # a.sample # => 3
+  # a.sample # => 1
+  # ```
+  #
+  # Uses the *random* instance when provided if the randomness needs to be
+  # controlled or to follow some traits. For example the following sample will
+  # always return the same value:
+  #
+  # ```
+  # a = [1, 2, 3]
+  # a.sample(Random.new(1)) # => 2
   # a.sample(Random.new(1)) # => 2
   # ```
-  def sample(random : Random = Random::DEFAULT)
+  def sample(random : Random? = nil)
     raise IndexError.new("Can't sample empty collection") if size == 0
-    unsafe_fetch(random.rand(size))
+    rng = random || Random.default
+    unsafe_fetch(rng.rand(size))
   end
 
   # :inherit:
   #
   # If `self` is not empty and `n` is equal to 1, calls `sample(random)` exactly
   # once. Thus, *random* will be left in a different state compared to the
   # implementation in `Enumerable`.
-  def sample(n : Int, random : Random = Random::DEFAULT) : Array(T)
+  def sample(n : Int, random : Random? = nil) : Array(T)
     return super unless n == 1
 
     if empty?

src/indexable/mutable.cr

@@ -251,17 +251,29 @@ module Indexable::Mutable(T)
     self
   end
 
-  # Modifies `self` by randomizing the order of elements in the collection
-  # using the given *random* number generator. Returns `self`.
+  # Modifies `self` by randomizing the order of elements in the collection.
+  # Returns `self`.
   #
   # ```
   # a = [1, 2, 3, 4, 5]
+  # a.shuffle! # => [3, 5, 2, 4, 1]
+  # a          # => [3, 5, 2, 4, 1]
+  # ```
+  #
+  # Uses the *random* instance when provided if the randomness needs to be
+  # controlled or to follow some traits. For example the following shuffle will
+  # always result in the same order:
+  #
+  # ```
+  # a = [1, 2, 3, 4, 5]
+  # a.shuffle!(Random.new(42)) # => [3, 2, 4, 5, 1]
   # a.shuffle!(Random.new(42)) # => [3, 2, 4, 5, 1]
   # a                          # => [3, 2, 4, 5, 1]
   # ```
-  def shuffle!(random : Random = Random::DEFAULT) : self
+  def shuffle!(random : Random? = nil) : self
+    rng = random || Random.default
     (size - 1).downto(1) do |i|
-      j = random.rand(i + 1)
+      j = rng.rand(i + 1)
       swap(i, j)
     end
     self

src/kernel.cr

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