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.
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
feat: free-threaded Python (3.13.0+) support #165
feat: free-threaded Python (3.13.0+) support #165
Changes from 11 commits
f335468
a1fb456
e93774a
e65f8ff
6a0f8fe
2cb3314
fe97888
ab38dcb
16ba89a
40ef816
a5718c1
9aa8250
752a0a7
e1f841b
0165921
31b9010
ccc3253
d972d96
4058638
173592d
eea87e7
c9d7bfa
1c51917
99f130a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see windows-specific and mac-specific code, but I don't see CI checks for the platforms. There do appear to be windows runners. https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners
It isn't the highest priority, but I am noting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are no longer using the GIL, then we should rename this function and instead have comments on why it is correct. If we are on 3.12 mode why are we still correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is made to fix a bug based on the following two observations:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmao-denver is right, there are several layers of questionable here, and the latest round here is my fault.
Why did it even test to begin with? Why does it clean up all other object any time any PyObject is about to be passed in to Java (instead of just once either just before or just after calling Java)? Couldn't say. I think removing the check is correct (either just before the ft patch, or as part of it), and we should reevaluate what is even happening here separately.
Agreed on renaming cleanupOnlyUseFromGIL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an oversight on my part during the "drop GIL" review.
I made the argument during the review that we might prefer to have some way to keep the GIL during "quick" java calls, or in other cases where we explicitly want to keep the GIL. I might argue that our call from
JType_CreateJavaPyObject
into the constructor ofPyObject
should keep the GIL.The reason we need this to only be called into the context of the GIL (historically) is because we need the GIL when calling
PyLib.decRef
; I'm assuming in the case of free-threaded (/GIL-less) builds, Py_DECREF is thread safe. And it's not only that Py_DECREF that needs to be thread-safe, the arbitrary python deallocation code that can be executed as part of a decRef to 0 needs to be thread-safe.In the context of free-threaded / GIL-less builds, maybe the functions need to be renamed to be something like
PyLib.isThreadSafe
/PyLib.ensureThreadSafe
, or something like that.