From 13bb8ccac4e26a649e12e774f5a6758dbd349129 Mon Sep 17 00:00:00 2001
From: Nathan Daly <NHDaly@gmail.com>
Date: Fri, 6 Sep 2019 05:36:49 -0400
Subject: [PATCH] Fix `assert_havelock(::ReentrantLock)` to assert that the
 _current-task_ has the lock. (#33159)

* Fix `assert_havelock(::ReentrantLock)` to assert that the _current-task_ has the lock.

Before this commit, new threads would incorrectly believe that they held
a lock on a Condition when they actually didn't, and would allow illegal
operations, e.g. notify:

```julia
julia> c = Threads.Condition()
Base.GenericCondition{ReentrantLock}(Base.InvasiveLinkedList{Task}(nothing, nothing), ReentrantLock(nothing, Base.GenericCondition{Base.Threads.SpinLock}(Base.InvasiveLinkedList{Task}(nothing, nothing), Base.Threads.SpinLock(Base.Threads.Atomic{Int64}(0))), 0))

julia> lock(c)

julia> fetch(Threads.@spawn Base.assert_havelock(c))  # This should be an ERROR (the new thread doesn't have the lock)

julia> fetch(Threads.@spawn notify(c))                # This should be an ERROR (the new thread doesn't have the lock)
0

julia> fetch(Threads.@spawn wait(c))                  # This error should be caught earlier (in assert_havelock).
ERROR: TaskFailedException:
unlock from wrong thread
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] unlockall(::ReentrantLock) at ./lock.jl:121
 [3] wait(::Base.GenericCondition{ReentrantLock}) at ./condition.jl:105
 [4] (::var"##19#20")() at ./threadingconstructs.jl:113
```

(The same holds for `@async` as `@spawn`.)

After this change, the assertion works correctly:
```
julia> c = Threads.Condition();

julia> lock(c)

julia> fetch(Threads.@spawn Base.assert_havelock(c))  # This correctly ERRORs
ERROR: TaskFailedException:
concurrency violation detected
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] concurrency_violation() at ./condition.jl:8
 [3] assert_havelock at ./condition.jl:28 [inlined]
 [4] assert_havelock at ./REPL[22]:1 [inlined]
 [5] assert_havelock(::Base.GenericCondition{ReentrantLock}) at ./condition.jl:73
 [6] (::var"##21#22")() at ./threadingconstructs.jl:113
```

Also adds unit test that failed before this commit but now succeeds

* Remove default impl of `assert_havelock`; add `::SpinLock` impl

(cherry picked from commit 784eb57a06aaab95bc4c22f288bd0c9f2b1687ca)
---
 base/condition.jl    |  1 -
 base/lock.jl         |  1 +
 base/locks-mt.jl     |  4 ++++
 test/threads_exec.jl | 15 +++++++++++++++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/base/condition.jl b/base/condition.jl
index 333075b7bfd6d..17d6b12f00df2 100644
--- a/base/condition.jl
+++ b/base/condition.jl
@@ -22,7 +22,6 @@ function trylock end
 function islocked end
 unlockall(l::AbstractLock) = unlock(l) # internal function for implementing `wait`
 relockall(l::AbstractLock, token::Nothing) = lock(l) # internal function for implementing `wait`
-assert_havelock(l::AbstractLock) = assert_havelock(l, Threads.threadid())
 assert_havelock(l::AbstractLock, tid::Integer) =
     (islocked(l) && tid == Threads.threadid()) ? nothing : concurrency_violation()
 assert_havelock(l::AbstractLock, tid::Task) =
diff --git a/base/lock.jl b/base/lock.jl
index 60066220b744e..ad728280c43e9 100644
--- a/base/lock.jl
+++ b/base/lock.jl
@@ -16,6 +16,7 @@ mutable struct ReentrantLock <: AbstractLock
     ReentrantLock() = new(nothing, GenericCondition{Threads.SpinLock}(), 0)
 end
 
+assert_havelock(l::ReentrantLock) = assert_havelock(l, l.locked_by)
 
 """
     islocked(lock) -> Status (Boolean)
diff --git a/base/locks-mt.jl b/base/locks-mt.jl
index 2ef1f8e2b2a67..a828adef1f913 100644
--- a/base/locks-mt.jl
+++ b/base/locks-mt.jl
@@ -30,6 +30,10 @@ struct SpinLock <: AbstractLock
     SpinLock() = new(Atomic{Int}(0))
 end
 
+# Note: this cannot assert that the lock is held by the correct thread, because we do not
+# track which thread locked it. Users beware.
+Base.assert_havelock(l::SpinLock) = islocked(l) ? nothing : concurrency_violation()
+
 function lock(l::SpinLock)
     while true
         if l.handle[] == 0
diff --git a/test/threads_exec.jl b/test/threads_exec.jl
index eb5ee45eb0972..2d6797fcf2600 100644
--- a/test/threads_exec.jl
+++ b/test/threads_exec.jl
@@ -151,6 +151,21 @@ end
 threaded_gc_locked(SpinLock)
 threaded_gc_locked(Threads.ReentrantLock)
 
+# Issue 33159
+# Make sure that a Threads.Condition can't be used without being locked, on any thread.
+@testset "Threads.Conditions must be locked" begin
+    c = Threads.Condition()
+    @test_throws Exception notify(c)
+    @test_throws Exception wait(c)
+
+    # If it's locked, but on the wrong thread, it should still throw an exception
+    lock(c)
+    @test_throws Exception fetch(@async notify(c))
+    @test_throws Exception fetch(@async notify(c, all=false))
+    @test_throws Exception fetch(@async wait(c))
+    unlock(c)
+end
+
 # Issue 14726
 # Make sure that eval'ing in a different module doesn't mess up other threads
 orig_curmodule14726 = @__MODULE__