Skip to content

Commit 9745986

Browse files
committed
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"#JuliaLang#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"#JuliaLang#21#22")() at ./threadingconstructs.jl:113 ``` Also adds unit test that failed before this commit but now succeeds
1 parent 921b4f8 commit 9745986

File tree

2 files changed

+16
-0
lines changed

2 files changed

+16
-0
lines changed

base/lock.jl

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ mutable struct ReentrantLock <: AbstractLock
1616
ReentrantLock() = new(nothing, GenericCondition{Threads.SpinLock}(), 0)
1717
end
1818

19+
assert_havelock(l::ReentrantLock) = assert_havelock(l, l.locked_by)
1920

2021
"""
2122
islocked(lock) -> Status (Boolean)

test/threads_exec.jl

+15
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,21 @@ end
151151
threaded_gc_locked(SpinLock)
152152
threaded_gc_locked(Threads.ReentrantLock)
153153

154+
# Issue 33159
155+
# Make sure that a Threads.Condition can't be used without being locked, on any thread.
156+
@testset "Threads.Conditions must be locked" begin
157+
c = Threads.Condition()
158+
@test_throws Exception notify(c)
159+
@test_throws Exception wait(c)
160+
161+
# If it's locked, but on the wrong thread, it should still throw an exception
162+
lock(c)
163+
@test_throws Exception fetch(@async notify(c))
164+
@test_throws Exception fetch(@async notify(c, all=false))
165+
@test_throws Exception fetch(@async wait(c))
166+
unlock(c)
167+
end
168+
154169
# Issue 14726
155170
# Make sure that eval'ing in a different module doesn't mess up other threads
156171
orig_curmodule14726 = @__MODULE__

0 commit comments

Comments
 (0)