Skip to content

Acquire GIL around every Python API call - #1097

Open
topolarity wants to merge 3 commits into
JuliaPy:masterfrom
topolarity:ct/with-gil
Open

topolarity wants to merge 3 commits into
JuliaPy:masterfrom
topolarity:ct/with-gil

Conversation

@topolarity

Copy link
Copy Markdown
Contributor

This should resolve #1096 by wrapping every Python API usage in a GIL acquire-release.

If this leads to overhead, we can probably re-export a "manual GIL" version of the API that allows consumers to manage the GIL themselves. In any case, it's worth having proper support for multi-threading.

Fixes #1096. Fixes #1095. Fixes #1090. Fixes #1088.
Fixes #1078. Fixes #1077. Fixes #1072. Fixes #1083.
Fixes timholy/Revise.jl#989.

@topolarity
topolarity force-pushed the ct/with-gil branch 5 times, most recently from b3aea39 to dd3cc6d Compare March 10, 2026 18:49
@topolarity

topolarity commented Mar 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Before this can be considered complete, it needs an equivalent to JuliaMath/FFTW.jl#160 to handle:

  1. Task A holds the GIL lock.
  2. Task A waits for Task B to make "progress".
  3. Task B waits in GC to obtain the GIL lock to run finalizers. (deadlocks)

edit: Done!

@topolarity
topolarity marked this pull request as draft March 10, 2026 19:16
@topolarity
topolarity force-pushed the ct/with-gil branch 4 times, most recently from 49be855 to bd8d220 Compare March 11, 2026 21:16
Majority of the API is covered by updating the `@pycheck(v/n)` macros.
The rest is covered manually by `@with_GIL`. Care should be taken to
avoid yielding to the Julia scheduler, which could cause task migration
and lead to deadlocks.
This is required to avoid deadlocking in the GC as it executes
finalizers.
Required to avoid starving the scheduler or deadlocking against the GC.
@yuyichao

yuyichao commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Is there any known issue with this currently? Are we going to use this approach?

I've rebased this on master at https://github.com/JuliaPy/PyCall.jl/tree/ct/with-gil-rebase with a few fixes/tweaks.

  1. pydecref_ needs to hold a reference to the julia object to avoid potential race with the GC. This is really hard to happen in practice but not impossible.

    As part of it, stops returning the pointer from it since that pointer should never be used again. This PR already changed the signature of this function. The only other reference I've found for this function is in some precompilation statements which shouldn't cause any breakage.

  2. Still make the finalizer clear the pointer in PyObject. I can't think of a case right now that would crash because of this but it would follow

  3. Minor optimization in _drain_release_queues to use one atomic sub rather than two, and to only copy the vector if it's not empty.

One of the few issues I could think of is maybe if there are a lot of object that became garbage they can only be free'd the next time the user calls a PyCall API, which may not happen for a while. For example, the following code can place unbound number of objects in the deferred list,

[PyObject(i) for i in 1:100000];

Maybe the finalizer should detect some cases where the GIL is safe to acquire (e.g. try to acquire without waiting) and clear the queue?

@yuyichao
yuyichao marked this pull request as ready for review October 2, 2026 17:29
@yuyichao

yuyichao commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

I've force pushed my rebased to the original branch. As I said in #1096 (comment), unless someone come up with a better solution soon, I'd like to merge this in a few days.

@codecov-commenter

codecov-commenter commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.85965% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.28%. Comparing base (2f600fb) to head (45ba68e).
⚠️ Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
src/conversions.jl 72.72% 3 Missing ⚠️
src/PyCall.jl 89.47% 2 Missing ⚠️
src/exception.jl 90.00% 1 Missing ⚠️
src/pyiterator.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1097      +/-   ##
==========================================
+ Coverage   68.02%   70.28%   +2.25%     
==========================================
  Files          20       21       +1     
  Lines        2036     1992      -44     
==========================================
+ Hits         1385     1400      +15     
+ Misses        651      592      -59     
Flag Coverage Δ
unittests 70.28% <93.85%> (+2.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@topolarity

Copy link
Copy Markdown
Contributor Author

Thanks for revisiting / rebasing @yuyichao

I am not aware of any problems with this, other than your latest fixups / tweaks. I think it's the direction we want to go in.

I'll try to reboot the context and take a look at the changes on Monday to see if we need anything else. Otherwise, sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment