Address review comments.

Co-authored-by: Nathan Daly <[email protected]>

src/fifolock.jl

@@ -5,7 +5,14 @@ const LOCKED_BIT = 0b01
 """
     FIFOLock()
 
-Like Base.ReentrantLock, but with strict FIFO ordering.
+A reentrant lock similar to Base.ReentrantLock, but with strict FIFO ordering.
+
+Base.ReentrantLock allows tasks to "barge the lock", i.e. it is occasionally
+possible for a task to jump the queue of tasks waiting for the lock; this is
+intentional behavior to increase throughput as described
+[here](https://webkit.org/blog/6161/locking-in-webkit/).
+
+When fairness is more important than throughput, use this FIFOLock.
 """
 mutable struct FIFOLock <: AbstractLock
     @atomic locked_by::Union{Task, Nothing}
@@ -83,8 +90,7 @@ end
     try
         _trylock(l, ct) && return
         wait(c)
-        l.reentrancy_cnt = 0x0000_0001
-        @atomic :release l.locked_by = ct
+        # l.locked_by and l.reentrancy_cnt are set in unlock
     finally
         unlock(c)
     end
@@ -127,6 +133,7 @@ end
                 t = popfirst!(c.waitq)
                 @atomic :release l.locked_by = t
                 schedule(t)
+                # Leave l.reentrancy_cnt at 1
             end
             GC.enable_finalizers()
         finally
@@ -138,6 +145,6 @@ end
     return
 end
 
-const Condition = Base.GenericCondition{FIFOLock}
+const FIFOCondition = Base.GenericCondition{FIFOLock}
 
 end

test/runtests.jl

@@ -230,34 +230,33 @@ end # @static if VERSION < v"1.8"
 @static if VERSION < v"1.10"
         @warn "skipping FIFOLock tests since VERSION ($VERSION) < v\"1.10\""
 else
+        curr = Threads.Atomic{Int}(1)
+        test_tasks = Task[]
+        sizehint!(test_tasks, 16)
+        tasks_in = zeros(Int, 16)
+        tasks_out = zeros(Int, 16)
+        tot = 0
         fl = FIFOLock()
         lock(fl)
-        tot = 0
-        ttsks = Task[]
-        tordr = Int[]
         try
-            # we assume here that the tasks spawned below run in the order
-            # they were spawned
             for i in 1:16
                 t = Threads.@spawn begin
+                    c = Threads.atomic_add!(curr, 1)
+                    tasks_in[i] = c
                     lock(fl)
                     try
                         tot = tot + 1
-                        if tot != i
-                            error("non-atomic access in lock")
-                        end
-                        push!(tordr, i)
+                        tasks_out[i] = c
                     finally
                         unlock(fl)
                     end
                 end
-                push!(ttsks, t)
-                sleep(0.1)
+                push!(test_tasks, t)
             end
         finally
             unlock(fl)
         end
-        for t in ttsks
+        for t in test_tasks
             @test try
                 wait(t)
                 true
@@ -266,7 +265,7 @@ else
             end
         end
         @test tot == 16
-        @test all([tordr[i] == i for i in 1:16])
+        @test tasks_out == tasks_in
 end # @static if VERSION < v"1.10"
     end