Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Sep 4, 2019

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> 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


(As I was experimenting with NHDaly/Select.jl#2, I noticed that this was missed for ReentrantLocks.)

…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"#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
@NHDaly NHDaly force-pushed the assert_havelock-ReentrantLock branch from 98639f9 to 9745986 Compare September 4, 2019 17:46
@JeffBezanson
Copy link
Member

Thanks, I believe this is a bugfix. I also think probably this method:

assert_havelock(l::AbstractLock) = assert_havelock(l, Threads.threadid())

should be removed since the current thread can't be generically assumed to be the lock holder. Then we can add a method for SpinLock that just checks islocked; we can't actually tell whether the current thread holds a SpinLock but that's better than nothing.

@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug multithreading Base.Threads and related functionality labels Sep 4, 2019
@NHDaly
Copy link
Member Author

NHDaly commented Sep 4, 2019

I agree. I was considering suggesting we remove the default-implementation, but wasn't confident in suggesting a larger refactoring.

+1 I'll make that change now. :) Thanks!

@NHDaly
Copy link
Member Author

NHDaly commented Sep 4, 2019

(Done)

@NHDaly NHDaly force-pushed the assert_havelock-ReentrantLock branch from 2675f88 to 59864d5 Compare September 4, 2019 20:33
@KristofferC KristofferC mentioned this pull request Sep 5, 2019
36 tasks
@KristofferC KristofferC merged commit 784eb57 into JuliaLang:master Sep 6, 2019
KristofferC pushed a commit that referenced this pull request Sep 6, 2019
…sk_ 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 784eb57)
@NHDaly NHDaly deleted the assert_havelock-ReentrantLock branch September 6, 2019 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants