Skip to content

Commit

Permalink
Fix assert_havelock(::ReentrantLock) to assert that the _current-ta…
Browse files Browse the repository at this point in the history
…sk_ 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
  • Loading branch information
NHDaly committed Sep 4, 2019
1 parent 921b4f8 commit 9745986
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 0 deletions.
1 change: 1 addition & 0 deletions base/lock.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
15 changes: 15 additions & 0 deletions test/threads_exec.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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__
Expand Down

0 comments on commit 9745986

Please sign in to comment.