threads: avoid deadlock from recursive lock acquire#38487
Merged
Conversation
7b9c0c0 to
8864d52
Compare
| function lock(l::SpinLock) | ||
| while true | ||
| if _get(l) == 0 | ||
| GC.enable_finalizers(false) |
Member
There was a problem hiding this comment.
Why can't this be done just before returning?
Member
Author
There was a problem hiding this comment.
The entire point of this bug fix is that it must be done before lock acquisition
Member
There was a problem hiding this comment.
For comparison, jl_mutex_lock only disables finalizers after jl_mutex_wait returns. I thought that would correspond to disabling them just after if p == 0 here.
Member
Author
There was a problem hiding this comment.
jl_mutex_lock can get away with doing potentially suspect things because it can ensure they are indistinguishable to the user
8864d52 to
112101a
Compare
JeffBezanson
approved these changes
Nov 23, 2020
maleadt
added a commit
to JuliaGPU/CUDA.jl
that referenced
this pull request
Nov 24, 2020
maleadt
approved these changes
Nov 24, 2020
Member
maleadt
left a comment
There was a problem hiding this comment.
Works fine with CUDA.jl (which had its own version of this).
maleadt
added a commit
to JuliaGPU/CUDA.jl
that referenced
this pull request
Nov 24, 2020
Member
|
The failure on Linux32 bit is related: |
112101a to
85ca118
Compare
Member
Author
|
Okay, I changed the implementation, as adding an error check for that revealed a number of violators. Should be happy now? |
85ca118 to
a4f8ea4
Compare
Member
Author
|
Okay, I changed the implementation, as that was not sufficient. Should be happy now? Unrelated, I also reduced the size of jl_task_t here by 2 pointers, to be consistent in the implementation. |
Member
Linux 64bit is still not happy. |
Member
Author
|
Okay, I changed the implementation, as I missed a line of code, and added some assertions for it. Should be happy now? Will squash now, as CI is green. |
fb9cfe9 to
64b71a5
Compare
vchuravy
approved these changes
Dec 1, 2020
JeffBezanson
reviewed
Dec 1, 2020
Finalizers can't safely acquire many essential locks (such as the iolock, to cleanup libuv objects) if they are run inside another lock. Therefore, inhibit all finalizers on the thread until all locks are released (previously, this was only true for our internal locks).
64b71a5 to
72bebdf
Compare
maleadt
added a commit
to JuliaGPU/CUDA.jl
that referenced
this pull request
Dec 7, 2020
vtjnash
added a commit
that referenced
this pull request
Dec 7, 2020
Finalizers can't safely acquire many essential locks (such as the iolock, to cleanup libuv objects) if they are run inside another lock. Therefore, inhibit all finalizers on the thread until all locks are released (previously, this was only true for our internal locks). (cherry-picked from 59aedd1)
vtjnash
added a commit
that referenced
this pull request
Dec 7, 2020
Finalizers can't safely acquire many essential locks (such as the iolock, to cleanup libuv objects) if they are run inside another lock. Therefore, inhibit all finalizers on the thread until all locks are released (previously, this was only true for our internal locks). (cherry-picked from 59aedd1)
vtjnash
added a commit
that referenced
this pull request
Dec 8, 2020
Finalizers can't safely acquire many essential locks (such as the iolock, to cleanup libuv objects) if they are run inside another lock. Therefore, inhibit all finalizers on the thread until all locks are released (previously, this was only true for our internal locks). (cherry-picked from 59aedd1)
vtjnash
added a commit
that referenced
this pull request
Dec 10, 2020
Finalizers can't safely acquire many essential locks (such as the iolock, to cleanup libuv objects) if they are run inside another lock. Therefore, inhibit all finalizers on the thread until all locks are released (previously, this was only true for our internal locks). (cherry-picked from 59aedd1)
maleadt
added a commit
to JuliaGPU/CUDA.jl
that referenced
this pull request
Jan 4, 2021
vchuravy
pushed a commit
to JuliaLang/Distributed.jl
that referenced
this pull request
Oct 6, 2023
…ia#38487) (JuliaLang/julia#38753) Finalizers can't safely acquire many essential locks (such as the iolock, to cleanup libuv objects) if they are run inside another lock. Therefore, inhibit all finalizers on the thread until all locks are released (previously, this was only true for our internal locks). (cherry-picked from ec8466b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finalizers can't safely acquire many essential locks (such as the
iolock, to cleanup libuv objects) if they are run inside another lock.
Therefore, inhibit all finalizers on the thread until all locks are
released (previously, this was only true for our internal locks).