Add error checks to Mutex, enabled by default (#8563)

spec/std/mutex_spec.cr

@@ -7,82 +7,137 @@ describe Mutex do
     mutex.unlock
   end
 
-  it "raises if unlocks without lock (reentrant)" do
-    mutex = Mutex.new(reentrant: true)
-    expect_raises(Exception, "Attempt to unlock a mutex which is not locked") do
-      mutex.unlock
+  it "works with multiple threads" do
+    x = 0
+    mutex = Mutex.new
+
+    fibers = 10.times.map do
+      spawn do
+        100.times do
+          mutex.synchronize { x += 1 }
+        end
+      end
+    end.to_a
+
+    fibers.each do |f|
+      while !f.dead?
+        Fiber.yield
+      end
     end
-  end
 
-  it "can be locked many times from the same fiber (reentrant)" do
-    mutex = Mutex.new(reentrant: true)
-    mutex.lock
-    mutex.lock
-    mutex.unlock
-    mutex.unlock
+    x.should eq(1000)
   end
 
-  it "can lock and unlock from multiple fibers" do
-    mutex = Mutex.new
+  describe "checked" do
+    it "raises if locked recursively" do
+      mutex = Mutex.new
+      mutex.lock
+      expect_raises(Exception, "Attempt to lock a mutex recursively (deadlock)") do
+        mutex.lock
+      end
+    end
 
-    a = 1
-    two = false
-    three = false
-    four = false
-    ch = Channel(Nil).new
+    it "raises if unlocks without lock" do
+      mutex = Mutex.new
+      expect_raises(Exception, "Attempt to unlock a mutex which is not locked") do
+        mutex.unlock
+      end
+    end
+
+    it "can't be unlocked by another fiber" do
+      mutex = nil
+
+      spawn do
+        mutex = Mutex.new
+        mutex.not_nil!.lock
+      end
 
-    spawn do
-      mutex.synchronize do
-        a = 2
+      until mutex
         Fiber.yield
-        two = a == 2
       end
-      ch.send(nil)
+
+      expect_raises(Exception, "Attempt to unlock a mutex locked by another fiber") do
+        mutex.not_nil!.unlock
+      end
     end
+  end
 
-    spawn do
-      mutex.synchronize do
-        a = 3
-        Fiber.yield
-        three = a == 3
+  describe "reentrant" do
+    it "can be locked many times from the same fiber" do
+      mutex = Mutex.new(:reentrant)
+      mutex.lock
+      mutex.lock
+      mutex.unlock
+      mutex.unlock
+    end
+
+    it "raises if unlocks without lock" do
+      mutex = Mutex.new(:reentrant)
+      expect_raises(Exception, "Attempt to unlock a mutex which is not locked") do
+        mutex.unlock
       end
-      ch.send(nil)
     end
 
-    spawn do
-      mutex.synchronize do
-        a = 4
+    it "can't be unlocked by another fiber" do
+      mutex = nil
+
+      spawn do
+        mutex = Mutex.new(:reentrant)
+        mutex.not_nil!.lock
+      end
+
+      until mutex
         Fiber.yield
-        four = a == 4
       end
-      ch.send(nil)
+
+      expect_raises(Exception, "Attempt to unlock a mutex locked by another fiber") do
+        mutex.not_nil!.unlock
+      end
     end
+  end
 
-    3.times { ch.receive }
+  describe "unchecked" do
+    it "can lock and unlock from multiple fibers" do
+      mutex = Mutex.new(:unchecked)
 
-    two.should be_true
-    three.should be_true
-    four.should be_true
-  end
+      a = 1
+      two = false
+      three = false
+      four = false
+      ch = Channel(Nil).new
 
-  it "works with multiple threads" do
-    x = 0
-    mutex = Mutex.new
+      spawn do
+        mutex.synchronize do
+          a = 2
+          Fiber.yield
+          two = a == 2
+        end
+        ch.send(nil)
+      end
 
-    fibers = 10.times.map do
       spawn do
-        100.times do
-          mutex.synchronize { x += 1 }
+        mutex.synchronize do
+          a = 3
+          Fiber.yield
+          three = a == 3
         end
+        ch.send(nil)
       end
-    end.to_a
 
-    fibers.each do |f|
-      while !f.dead?
-        Fiber.yield
+      spawn do
+        mutex.synchronize do
+          a = 4
+          Fiber.yield
+          four = a == 4
+        end
+        ch.send(nil)
       end
-    end
 
-    x.should eq(1000)
+      3.times { ch.receive }
+
+      two.should be_true
+      three.should be_true
+      four.should be_true
+    end
   end
 end

src/crystal/once.cr

@@ -12,7 +12,7 @@
 class Crystal::OnceState
   @rec = [] of Bool*
   {% if flag?(:preview_mt) %}
-    @mutex = Mutex.new(reentrant: true)
+    @mutex = Mutex.new(:reentrant)
   {% end %}
 
   def once(flag : Bool*, initializer : Void*)

src/logger.cr

@@ -121,7 +121,7 @@ class Logger
   # If *io* is `nil` then all log calls will be silently ignored.
   def initialize(@io : IO?, @level = Severity::INFO, @formatter = DEFAULT_FORMATTER, @progname = "")
     @closed = false
-    @mutex = Mutex.new
+    @mutex = Mutex.new(:unchecked)
   end
 
   # Calls the *close* method on the object passed to `initialize`.

src/mutex.cr

@@ -1,6 +1,20 @@
 require "crystal/spin_lock"
 
 # A fiber-safe mutex.
+#
+# Provides deadlock protection by default. Attempting to re-lock the mutex from
+# the same fiber will raise an exception. Trying to unlock an unlocked mutex, or
+# a mutex locked by another fiber will raise an exception.
+#
+# The reentrant protection maintains a lock count. Attempting to re-lock the
+# mutex from the same fiber will increment the lock count. Attempting to unlock
+# the counter from the same fiber will decrement the lock count, eventually
+# releasing the lock when the lock count returns to 0. Attempting to unlock an
+# unlocked mutex, or a mutex locked by another fiber will raise an exception.
+#
+# You also disable all protections with `unchecked`. Attempting to re-lock the
+# mutex from the same fiber will deadlock. Any fiber can unlock the mutex, even
+# if it wasn't previously locked.
 class Mutex
   @state = Atomic(Int32).new(0)
   @mutex_fiber : Fiber?
@@ -9,19 +23,29 @@ class Mutex
   @queue_count = Atomic(Int32).new(0)
   @lock = Crystal::SpinLock.new
 
-  def initialize(*, @reentrant = false)
+  enum Protection
+    Checked
+    Reentrant
+    Unchecked
+  end
+
+  def initialize(@protection : Protection = :checked)
   end
 
   @[AlwaysInline]
   def lock
     if @state.swap(1) == 0
-      @mutex_fiber = Fiber.current if @reentrant
+      @mutex_fiber = Fiber.current unless @protection.unchecked?
       return
     end
 
-    if @reentrant && @mutex_fiber == Fiber.current
-      @lock_count += 1
-      return
+    if !@protection.unchecked? && @mutex_fiber == Fiber.current
+      if @protection.reentrant?
+        @lock_count += 1
+        return
+      else
+        raise "Attempt to lock a mutex recursively (deadlock)"
+      end
     end
 
     lock_slow
@@ -66,12 +90,16 @@ class Mutex
   end
 
   def unlock
-    if @reentrant
-      unless @mutex_fiber == Fiber.current
+    unless @protection.unchecked?
+      if mutex_fiber = @mutex_fiber
+        unless mutex_fiber == Fiber.current
+          raise "Attempt to unlock a mutex locked by another fiber"
+        end
+      else
         raise "Attempt to unlock a mutex which is not locked"
       end
 
-      if @lock_count > 0
+      if @protection.reentrant? && @lock_count > 0
         @lock_count -= 1
         return
       end

src/signal.cr

@@ -147,7 +147,7 @@ module Crystal::Signal
   @@pipe = IO.pipe(read_blocking: false, write_blocking: true)
   @@handlers = {} of ::Signal => Handler
   @@child_handler : Handler?
-  @@mutex = Mutex.new
+  @@mutex = Mutex.new(:unchecked)
 
   def self.trap(signal, handler) : Nil
     @@mutex.synchronize do
@@ -272,7 +272,7 @@ module Crystal::SignalChildHandler
 
   @@pending = {} of LibC::PidT => Int32
   @@waiting = {} of LibC::PidT => Channel(Int32)
-  @@mutex = Mutex.new
+  @@mutex = Mutex.new(:unchecked)
 
   def self.wait(pid : LibC::PidT) : Channel(Int32)
     channel = Channel(Int32).new(1)