From 943473fe090f9bddde537fda3e6d2e226eb5b6f7 Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Sun, 6 Jul 2025 23:37:13 -0400 Subject: [PATCH 1/7] Surround the GIL with a ReentrantLock on the Julia side. The Python GIL should only be able obtained by a single Julia thread / task. To enforce this, we use a ReentrantLock on the Julia side that must be obtained before trying to acquire the Python GIL. --- src/GIL/GIL.jl | 65 ++++++++++++++++++++++++++++++++++++++++---------- test/GC.jl | 9 +++++++ test/GIL.jl | 17 +++++++++++++ 3 files changed, 79 insertions(+), 12 deletions(-) diff --git a/src/GIL/GIL.jl b/src/GIL/GIL.jl index 2ff90c12..fe0ab75a 100644 --- a/src/GIL/GIL.jl +++ b/src/GIL/GIL.jl @@ -9,6 +9,18 @@ module GIL using ..C: C +# Ensure that only one Julia thread tries to acquire the Python GIL +# PyGILState_Ensure and PyGILState_Release may not be thread safe. +# https://github.com/JuliaPy/PythonCall.jl/issues/627 +const _jl_gil_lock = ReentrantLock() + +""" + hasgil() + +Returns `true` if the current thread has the GIL or `false` otherwise. +""" +hasgil() = C.PyGILState_Check() == Cint(1) + """ lock(f) @@ -21,11 +33,16 @@ threads. Since the main Julia thread holds the GIL by default, you will need to See [`@lock`](@ref) for the macro form. """ function lock(f) - state = C.PyGILState_Ensure() + Base.lock(_jl_gil_lock) try - f() + state = C.PyGILState_Ensure() + try + f() + finally + C.PyGILState_Release(state) + end finally - C.PyGILState_Release(state) + Base.unlock(_jl_gil_lock) end end @@ -42,11 +59,16 @@ The macro equivalent of [`lock`](@ref). """ macro lock(expr) quote - state = C.PyGILState_Ensure() + Base.lock(_jl_gil_lock) try - $(esc(expr)) + state = C.PyGILState_Ensure() + try + $(esc(expr)) + finally + C.PyGILState_Release(state) + end finally - C.PyGILState_Release(state) + Base.unlock(_jl_gil_lock) end end end @@ -63,11 +85,17 @@ Python code. That other thread can be a Julia thread, which must lock the GIL us See [`@unlock`](@ref) for the macro form. """ function unlock(f) - state = C.PyEval_SaveThread() + _locked = Base.islocked(_jl_gil_lock) + _locked && Base.unlock(_jl_gil_lock) try - f() + state = C.PyEval_SaveThread() + try + f() + finally + C.PyEval_RestoreThread(state) + end finally - C.PyEval_RestoreThread(state) + _locked && Base.lock(_jl_gil_lock) end end @@ -84,13 +112,26 @@ The macro equivalent of [`unlock`](@ref). """ macro unlock(expr) quote - state = C.PyEval_SaveThread() + _locked = Base.islocked(_jl_gil_lock) + _locked && Base.unlock(_jl_gil_lock) try - $(esc(expr)) + state = C.PyEval_SaveThread() + try + $(esc(expr)) + finally + C.PyEval_RestoreThread(state) + end finally - C.PyEval_RestoreThread(state) + _locked && Base.lock(_jl_gil_lock) end end end +# If the main thread already has the GIL, we should lock _jl_gil_lock. +function __init__() + if hasgil() + Base.lock(_jl_gil_lock) + end +end + end diff --git a/test/GC.jl b/test/GC.jl index 475c547c..c04fab61 100644 --- a/test/GC.jl +++ b/test/GC.jl @@ -1,4 +1,7 @@ +using TestItemRunner + @testitem "GC.gc()" begin + using PythonCall let pyobjs = map(pylist, 1:100) PythonCall.GIL.@unlock Threads.@threads for obj in pyobjs @@ -13,6 +16,7 @@ end @testitem "GC.GCHook" begin + using PythonCall let pyobjs = map(pylist, 1:100) PythonCall.GIL.@unlock Threads.@threads for obj in pyobjs @@ -23,5 +27,10 @@ end VERSION >= v"1.10.0-" && @test !isempty(PythonCall.GC.QUEUE.items) GC.gc() + + # Unlock and relocking the ReentrantLock allows this test to pass + Base.unlock(PythonCall.GIL._jl_gil_lock) + Base.lock(PythonCall.GIL._jl_gil_lock) + @test isempty(PythonCall.GC.QUEUE.items) end diff --git a/test/GIL.jl b/test/GIL.jl index ca1f6405..10cf059e 100644 --- a/test/GIL.jl +++ b/test/GIL.jl @@ -18,6 +18,14 @@ if Threads.nthreads() ≥ 2 @test 0.9 < t.time < 1.2 end + + @test PythonCall.GIL.hasgil() + PythonCall.GIL.unlock() do + @test !Base.islocked(PythonCall.GIL._jl_gil_lock) + PythonCall.GIL.lock() do + @test Base.islocked(PythonCall.GIL._jl_gil_lock) + end + end end @testitem "@unlock and @lock" begin @@ -36,4 +44,13 @@ end if Threads.nthreads() ≥ 2 @test 0.9 < t.time < 1.2 end + + @test PythonCall.GIL.hasgil() + PythonCall.GIL.@unlock begin + @test !Base.islocked(PythonCall.GIL._jl_gil_lock) + PythonCall.GIL.@lock begin + @test Base.islocked(PythonCall.GIL._jl_gil_lock) + end + end + end From 3b6caaa0a03ea4996dab462ec44bbf5262040a5b Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Mon, 7 Jul 2025 01:32:46 -0400 Subject: [PATCH 2/7] Change explanation to Jameson Nash (vtjnash)'s reasoning --- src/GIL/GIL.jl | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/GIL/GIL.jl b/src/GIL/GIL.jl index fe0ab75a..c02e2b81 100644 --- a/src/GIL/GIL.jl +++ b/src/GIL/GIL.jl @@ -9,8 +9,10 @@ module GIL using ..C: C -# Ensure that only one Julia thread tries to acquire the Python GIL -# PyGILState_Ensure and PyGILState_Release may not be thread safe. +# Ensure that only one Julia task tries to acquire the Python GIL. +# Avoid the potential issue that a task could miscompute whether +# it actually has the GIL simply because a different task that ran +# on the same thread that once had the GIL. # https://github.com/JuliaPy/PythonCall.jl/issues/627 const _jl_gil_lock = ReentrantLock() From beea5f20d0e5f9473bea6b21a235fbfa9b85cf32 Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Mon, 7 Jul 2025 01:33:37 -0400 Subject: [PATCH 3/7] Add an exclusive parameter to GIL.lock and GIL.@lock Setting the exclusive parameter to false allow for the old behavior of only depending on Python's GIL mechanisms. Setting the exclusive parameter to false bypasses the ReentrantLock `GIL._jl_gil_lock`. --- src/GIL/GIL.jl | 41 ++++++++++++++++++++++++++++++++++++----- test/GIL.jl | 4 ++-- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/GIL/GIL.jl b/src/GIL/GIL.jl index c02e2b81..cf609a70 100644 --- a/src/GIL/GIL.jl +++ b/src/GIL/GIL.jl @@ -24,7 +24,7 @@ Returns `true` if the current thread has the GIL or `false` otherwise. hasgil() = C.PyGILState_Check() == Cint(1) """ - lock(f) + lock(f; exclusive=true) Lock the GIL, compute `f()`, unlock the GIL, then return the result of `f()`. @@ -32,10 +32,15 @@ Use this to run Python code from threads that do not currently hold the GIL, suc threads. Since the main Julia thread holds the GIL by default, you will need to [`unlock`](@ref) the GIL before using this function. +Setting the `exclusive` keyword to `false` allows more than one Julia `Task` +to attempt to acquire the GIL. This may be useful if one Julia `Task` calls +code which releases the GIL, in which case another Julia task could acquire +it. However, this is probably not a sound approach. + See [`@lock`](@ref) for the macro form. """ -function lock(f) - Base.lock(_jl_gil_lock) +function lock(f; exclusive=true) + exclusive && Base.lock(_jl_gil_lock) try state = C.PyGILState_Ensure() try @@ -44,12 +49,12 @@ function lock(f) C.PyGILState_Release(state) end finally - Base.unlock(_jl_gil_lock) + exclusive && Base.unlock(_jl_gil_lock) end end """ - @lock expr + @lock [exclusive=true] expr Lock the GIL, compute `expr`, unlock the GIL, then return the result of `expr`. @@ -57,6 +62,11 @@ Use this to run Python code from threads that do not currently hold the GIL, suc threads. Since the main Julia thread holds the GIL by default, you will need to [`@unlock`](@ref) the GIL before using this function. +Setting the `exclusive` parameter to `false` allows more than one Julia `Task` +to attempt to acquire the GIL. This may be useful if one Julia `Task` calls +code which releases the GIL, in which case another Julia task could acquire +it. However, this is probably not a sound approach. + The macro equivalent of [`lock`](@ref). """ macro lock(expr) @@ -75,6 +85,27 @@ macro lock(expr) end end +macro lock(parameter, expr) + parameter.head == :(=) && + parameter.args[1] == :exclusive || + throw(ArgumentError("The only accepted parameter to @lock is `exclusive`.")) + + do_lock = esc(parameter.args[2]) + quote + $do_lock && Base.lock(_jl_gil_lock) + try + state = C.PyGILState_Ensure() + try + $(esc(expr)) + finally + C.PyGILState_Release(state) + end + finally + $do_lock && Base.unlock(_jl_gil_lock) + end + end +end + """ unlock(f) diff --git a/test/GIL.jl b/test/GIL.jl index 10cf059e..a664bf6a 100644 --- a/test/GIL.jl +++ b/test/GIL.jl @@ -4,7 +4,7 @@ function threaded_sleep() PythonCall.GIL.unlock() do Threads.@threads for i = 1:2 - PythonCall.GIL.lock() do + PythonCall.GIL.lock(exclusive=false) do pyimport("time").sleep(1) end end @@ -33,7 +33,7 @@ end # GIL, these can happen in parallel if Julia has at least 2 threads. function threaded_sleep() PythonCall.GIL.@unlock Threads.@threads for i = 1:2 - PythonCall.GIL.@lock pyimport("time").sleep(1) + PythonCall.GIL.@lock exclusive=false pyimport("time").sleep(1) end end # one run to ensure it's compiled From 2f795dc2ca2bc214c4b61ed845999ff7082a6093 Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Mon, 7 Jul 2025 02:48:26 -0400 Subject: [PATCH 4/7] Do not lock on __init__ --- .gitignore | 4 ++++ src/GIL/GIL.jl | 3 +++ test/GC.jl | 5 +++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 3bed89d2..2ad921bf 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,7 @@ dist/ .CondaPkg/ /jltest.* uv.lock + +# pixi environments +.pixi +*.egg-info diff --git a/src/GIL/GIL.jl b/src/GIL/GIL.jl index cf609a70..3a4ca625 100644 --- a/src/GIL/GIL.jl +++ b/src/GIL/GIL.jl @@ -160,11 +160,14 @@ macro unlock(expr) end end +#= +# Disable this for now since holding this lock will disable finalizers # If the main thread already has the GIL, we should lock _jl_gil_lock. function __init__() if hasgil() Base.lock(_jl_gil_lock) end end +=# end diff --git a/test/GC.jl b/test/GC.jl index c04fab61..b62fec82 100644 --- a/test/GC.jl +++ b/test/GC.jl @@ -29,8 +29,9 @@ end GC.gc() # Unlock and relocking the ReentrantLock allows this test to pass - Base.unlock(PythonCall.GIL._jl_gil_lock) - Base.lock(PythonCall.GIL._jl_gil_lock) + # if _jl_gil_lock is locked on init + # Base.unlock(PythonCall.GIL._jl_gil_lock) + # Base.lock(PythonCall.GIL._jl_gil_lock) @test isempty(PythonCall.GC.QUEUE.items) end From 53867da4f0d75d67f6faf82a1ffa8556fd1a421b Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Tue, 8 Jul 2025 11:50:47 -0400 Subject: [PATCH 5/7] Add initial draft of GlobalInterpreterLock --- src/GIL/GIL.jl | 2 + src/GIL/GlobalInterpreterLock.jl | 120 +++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 src/GIL/GlobalInterpreterLock.jl diff --git a/src/GIL/GIL.jl b/src/GIL/GIL.jl index 3a4ca625..28e2ec59 100644 --- a/src/GIL/GIL.jl +++ b/src/GIL/GIL.jl @@ -170,4 +170,6 @@ function __init__() end =# +include("GlobalInterpreterLock.jl") + end diff --git a/src/GIL/GlobalInterpreterLock.jl b/src/GIL/GlobalInterpreterLock.jl new file mode 100644 index 00000000..a3adbfe8 --- /dev/null +++ b/src/GIL/GlobalInterpreterLock.jl @@ -0,0 +1,120 @@ +struct TaskState + task::Task + sticky::Bool # original stickiness of the task + state::C.PyGILState_STATE +end + +struct TaskStack + stack::Vector{TaskState} + count::IdDict{Task,Int} + condvar::Threads.Condition + function TaskStack() + return new(TaskState[], IdDict{Task,Int}(), Threads.Condition()) + end +end +function Base.last(task_stack::TaskStack)::Task + return last(task_stack.stack).task +end +function Base.push!(task_stack::TaskStack, task::Task) + original_sticky = task.sticky + # The task should not migrate threads while acquiring or holding the GIL + task.sticky = true + gil_state = C.PyGILState_Ensure() + + # Save the stickiness and state for when we release + state = TaskState(task, original_sticky, gil_state) + push!(task_stack.stack, state) + + # Increment the count for this task + count = get(task_stack.count, task, 0) + task_stack.count[task] = count + 1 + + return task_stack +end +function Base.pop!(task_stack::TaskStack)::Task + state = pop!(task_stack.stack) + task = state.task + sticky = state.sticky + gil_state = state.state + + # Decrement the count for this task + count = task_stack.count[task] - 1 + if count == 0 + # If 0, remove it from the key set + pop!(task_stack.count, task) + else + task_stack[task] = count + end + + C.PyGILState_Release(gil_state) + + # Restore sticky state after releasing the GIL + task.sticky = sticky + + Base.lock(task_stack.condvar) do + notify(task_stack.condvar) + end + + return task +end +Base.isempty(task_stack::TaskStack) = isempty(task_stack.stack) + +if !isdefined(Base, :OncePerThread) + + # OncePerThread is implemented in full in Julia 1.12 + # This implementation is meant for compatibility with Julia 1.10 and 1.11 + # and only supports a static number of threads. Use Julia 1.12 for dynamic + # thread usage. + mutable struct OncePerThread{T,F} <: Function + @atomic xs::Vector{T} # values + @atomic ss::Vector{UInt8} # states: 0=initial, 1=hasrun, 2=error, 3==concurrent + const initializer::F + function OncePerThread{T,F}(initializer::F) where {T,F} + nt = Threads.maxthreadid() + return new{T,F}(Vector{T}(undef, nt), zeros(UInt8, nt), initializer) + end + end + OncePerThread{T}(initializer::Type{U}) where {T, U} = OncePerThread{T,Type{U}}(initializer) + (once::OncePerThread{T,F})() where {T,F} = once[Threads.threadid()] + function Base.getindex(once::OncePerThread, tid::Integer) + tid = Threads.threadid() + ss = @atomic :acquire once.ss + xs = @atomic :monotonic once.xs + if checkbounds(Bool, xs, tid) + if ss[tid] == 0 + xs[tid] = once.initializer() + ss[tid] = 1 + end + return xs[tid] + else + throw(ErrorException("Thread id $tid is out of bounds as initially allocated. Use Julia 1.12 for dynamic thread usage.")) + end + end + +end + +struct GlobalInterpreterLock <: Base.AbstractLock + lock_owners::OncePerThread{TaskStack} + function GlobalInterpreterLock() + return new(OncePerThread{TaskStack}(TaskStack)) + end +end +function Base.lock(gil::GlobalInterpreterLock) + push!(gil.lock_owners(), current_task()) + return nothing +end +function Base.unlock(gil::GlobalInterpreterLock) + lock_owner::TaskStack = gil.lock_owners() + while last(lock_owner) != current_task() + wait(lock_owner.condvar) + end + task = pop!(lock_owner) + @assert task == current_task() + return nothing +end +function Base.islocked(gil::GlobalInterpreterLock) + # TODO: handle Julia 1.10 and 1.11 case when have not allocated up to maxthreadid + return any(!isempty(gil.lock_owners[thread_index]) for thread_index in 1:Threads.maxthreadid()) +end + +const _GIL = GlobalInterpreterLock() From 58dba62efb2f663b54e494bd939eedaf0d118eef Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Thu, 10 Jul 2025 23:19:30 -0400 Subject: [PATCH 6/7] First GlobalInterpreterLock that works --- src/GIL/GlobalInterpreterLock.jl | 92 +++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 18 deletions(-) diff --git a/src/GIL/GlobalInterpreterLock.jl b/src/GIL/GlobalInterpreterLock.jl index a3adbfe8..ffa07340 100644 --- a/src/GIL/GlobalInterpreterLock.jl +++ b/src/GIL/GlobalInterpreterLock.jl @@ -1,9 +1,23 @@ +""" + TaskState + +When a `Task` acquires the GIL, save the GIL state and the stickiness of the +`Task` since we will force the `Task` to be sticky. We need to restore the GIL +state on release of the GIL via `C.PyGILState_Release`. +""" struct TaskState task::Task sticky::Bool # original stickiness of the task state::C.PyGILState_STATE end +""" + TaskStack + +For each thread the `TaskStack` maintains a first-in-last-out list of tasks +as well as the GIL state and their stickiness upon entering the stack. This +forces tasks to unlock the GIL in the reverse order of which they locked it. +""" struct TaskStack stack::Vector{TaskState} count::IdDict{Task,Int} @@ -43,7 +57,7 @@ function Base.pop!(task_stack::TaskStack)::Task # If 0, remove it from the key set pop!(task_stack.count, task) else - task_stack[task] = count + task_stack.count[task] = count end C.PyGILState_Release(gil_state) @@ -57,42 +71,56 @@ function Base.pop!(task_stack::TaskStack)::Task return task end +Base.in(task::Task, task_stack::TaskStack) = haskey(task_stack.count) Base.isempty(task_stack::TaskStack) = isempty(task_stack.stack) if !isdefined(Base, :OncePerThread) + const PerThreadLock = Base.ThreadSynchronizer() + # OncePerThread is implemented in full in Julia 1.12 - # This implementation is meant for compatibility with Julia 1.10 and 1.11 - # and only supports a static number of threads. Use Julia 1.12 for dynamic - # thread usage. + # This implementation is meant for compatibility with Julia 1.10 and 1.11. + # Using Julia 1.12 is recommended. mutable struct OncePerThread{T,F} <: Function - @atomic xs::Vector{T} # values - @atomic ss::Vector{UInt8} # states: 0=initial, 1=hasrun, 2=error, 3==concurrent + @atomic xs::Dict{Int, T} # values + @atomic ss::Dict{Int, UInt8} # states: 0=initial, 1=hasrun, 2=error, 3==concurrent const initializer::F function OncePerThread{T,F}(initializer::F) where {T,F} nt = Threads.maxthreadid() - return new{T,F}(Vector{T}(undef, nt), zeros(UInt8, nt), initializer) + return new{T,F}(Dict{Int,T}(), Dict{Int,UInt8}(), initializer) end end OncePerThread{T}(initializer::Type{U}) where {T, U} = OncePerThread{T,Type{U}}(initializer) (once::OncePerThread{T,F})() where {T,F} = once[Threads.threadid()] function Base.getindex(once::OncePerThread, tid::Integer) - tid = Threads.threadid() + tid = Int(tid) ss = @atomic :acquire once.ss xs = @atomic :monotonic once.xs - if checkbounds(Bool, xs, tid) - if ss[tid] == 0 + + if haskey(ss, tid) && ss[tid] == 1 + return xs[tid] + end + + Base.lock(PerThreadLock) + try + state = get(ss, tid, 0) + if state == 0 xs[tid] = once.initializer() ss[tid] = 1 end - return xs[tid] - else - throw(ErrorException("Thread id $tid is out of bounds as initially allocated. Use Julia 1.12 for dynamic thread usage.")) + finally + Base.unlock(PerThreadLock) end + return xs[tid] end - end +""" + GlobalInterpreterLock + +Provides a thread aware reentrant lock around Python's interpreter lock that +ensures that `Task`s acquiring the lock stay on the same thread. +""" struct GlobalInterpreterLock <: Base.AbstractLock lock_owners::OncePerThread{TaskStack} function GlobalInterpreterLock() @@ -105,16 +133,44 @@ function Base.lock(gil::GlobalInterpreterLock) end function Base.unlock(gil::GlobalInterpreterLock) lock_owner::TaskStack = gil.lock_owners() - while last(lock_owner) != current_task() - wait(lock_owner.condvar) + last_owner::Task = if isempty(lock_owner) + current_task() + else + last(lock_owner) + end + while last_owner != current_task() + if istaskdone(last_owner) && !isempty(lock_owner) + # Last owner is done and unable to unlock the GIL + pop!(lock_owner) + error("Unlock from the wrong task. The Task that owned the GIL is done and did not unlock the GIL: $(last_owner)") + else + # This task does not own the GIL. Wait to unlock the GIL until + # another task successfully unlocks the GIL. + wait(lock_owner.condvar) + end + last_owner = if isempty(lock_owner) + current_task() + else + last(lock_owner) + end + end + if isempty(lock_owner) + error("Unlock from wrong task: $(current_task). No tasks on this thread own the lock.") + else + task = pop!(lock_owner) end - task = pop!(lock_owner) @assert task == current_task() return nothing end function Base.islocked(gil::GlobalInterpreterLock) - # TODO: handle Julia 1.10 and 1.11 case when have not allocated up to maxthreadid return any(!isempty(gil.lock_owners[thread_index]) for thread_index in 1:Threads.maxthreadid()) end +function haslock(gil::GlobalInterpreterLock, task::Task) + lock_owner::TaskStack = gil.lock_owners() + if isempty(lock_owner) + return false + end + return last(lock_owner)::Task == task +end const _GIL = GlobalInterpreterLock() From 3b97f9c05d629cf5f4f0cd2d112f3bd98a337a04 Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Fri, 11 Jul 2025 02:22:56 -0400 Subject: [PATCH 7/7] Lock condvar to wait, restore sticky after notify --- src/GIL/GlobalInterpreterLock.jl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/GIL/GlobalInterpreterLock.jl b/src/GIL/GlobalInterpreterLock.jl index ffa07340..332f4772 100644 --- a/src/GIL/GlobalInterpreterLock.jl +++ b/src/GIL/GlobalInterpreterLock.jl @@ -62,13 +62,13 @@ function Base.pop!(task_stack::TaskStack)::Task C.PyGILState_Release(gil_state) - # Restore sticky state after releasing the GIL - task.sticky = sticky - Base.lock(task_stack.condvar) do notify(task_stack.condvar) end + # Restore sticky state after releasing the GIL + task.sticky = sticky + return task end Base.in(task::Task, task_stack::TaskStack) = haskey(task_stack.count) @@ -146,7 +146,9 @@ function Base.unlock(gil::GlobalInterpreterLock) else # This task does not own the GIL. Wait to unlock the GIL until # another task successfully unlocks the GIL. - wait(lock_owner.condvar) + Base.lock(lock_owner.condvar) do + wait(lock_owner.condvar) + end end last_owner = if isempty(lock_owner) current_task()