Skip to content

Surround the GIL with a ReentrantLock on the Julia side. #637

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Surround the GIL with a ReentrantLock on the Julia side. #637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
943473f
Surround the GIL with a ReentrantLock on the Julia side.
mkitti Jul 7, 2025
3b6caaa
Change explanation to Jameson Nash (vtjnash)'s reasoning
mkitti Jul 7, 2025
beea5f2
Add an exclusive parameter to GIL.lock and GIL.@lock
mkitti Jul 7, 2025
2f795dc
Do not lock on __init__
mkitti Jul 7, 2025
53867da
Add initial draft of GlobalInterpreterLock
mkitti Jul 8, 2025
58dba62
First GlobalInterpreterLock that works
mkitti Jul 11, 2025
3b97f9c
Lock condvar to wait, restore sticky after notify
mkitti Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 53 additions & 12 deletions src/GIL/GIL.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reasoning is nonsense since those functions are explicitly thread safe. There may be good reasons for this PR, but this is not it.

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

Expand All @@ -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

Expand All @@ -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
Expand All @@ -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

Expand All @@ -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
Comment on lines +166 to +170
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locking on __init__ is probably a bad idea even though we are technically holding the GIL since this will disable finalizers.

Rather we should consider invoking lock while executing other calls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling this apparently allowed the python tests to succeed.

Copy link

@vtjnash vtjnash Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sleeping on this, I propose you make the following changes to make this fix work:

  • Replace this ReentrantLock with a OncePerThread{@NamedTuple{stack::Vector{Task}, set::IdSet{Task}, condvar::Condition}} lockowners. The point being mostly to convert the state on ReentrantLock into something similar but protected by the GIL to avoid concurrency, finalizer, and multi-threading issues.
  • Perhaps, after locking the GIL on this thread, check that the current Task is in(lockowners().set, current_task) for safety (forcing the user to make explicit GIL usage on each Task with a call to lock on each Task)?
  • During the calls to lock (after getting GIL), push the current Task to the lockowners stack and add it to the set (if using that)
  • On unlock, block (on condvar) until the current_task is at the top of the stack (notify condvar after every unlock) to enforce that GIL unlock calls are correctly sequenced
  • Where necessary (anywhere internally that is holding the GIL to update a Julia data structure related to python), add calls to enable/disable finalizers to prevent memory corruption from concurrent mutation of the data structures (separate PR)
  • Whenever a Task interacts with python, force current_task().sticky = true to prevent it migrating to a thread that is not holding the GIL.
  • Adding __init__ is dangerous since that doesn't run on the main Task in some user applications even though it always does so during your CI tests. You might just want to add some special exemptions for when current_task() == root_task(0)?

Does that all sound logical and reasonable? My proposal already went through a few drafts as I tried to make it seem straightforward enough to explain and use, so it might not be the only way to deal with some of these constraints, and I'm probably forgetting at least some important details.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OncePerThread is to be introduced in Julia 1.12 which has yet to be released at the time of this writing. Is there a lesser version of this that could be implemented prior to Julia 1.12, preferably to Julia 1.10 LTS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vtjnash Should Condition here be Base.Condition (Base.GenericCondition{Base.AlwaysLockedST}) or Threads.Condition (Base.GenericCondition{ReentrantLock}) ?

I originally thought you meant Threads.Condition, but now I'm thinking Base.Condition may be correct since in theory all of the Tasks waiting to unlock should already be on the same thread.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Base.Condition, since this is locked to thread-local state


end
9 changes: 9 additions & 0 deletions test/GC.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand All @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vtjnash If I remove this unlock and lock this test fails. Does this invoke the GC again somehow or is there some concurrency issue that prevents this task from realizing that the QUEUE.items is actually empty.

QUEUE is defined here:

PythonCall.jl/src/GC/GC.jl

Line 12 in c84c059

const QUEUE = (; items = C.PyPtr[], lock = Threads.SpinLock())

QUEUE.items is initialized as C.PyPtr[]

In this test, it should be emptied by this call in the finalizer of GCHook:

PythonCall.jl/src/GC/GC.jl

Lines 142 to 144 in c84c059

if !isempty(QUEUE.items) && C.PyGILState_Check() == 1
unsafe_free_queue()
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finalizers must be disabled when any locks are held to avoid interrupting a critical section and causing memory corruption


@test isempty(PythonCall.GC.QUEUE.items)
end
17 changes: 17 additions & 0 deletions test/GIL.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Loading