gh-142476: fix memory leak when creating JIT executors #142492
Merged
+9
−2
Conversation
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
ashm-dev
changed the title
Contributor
Author
|
I believe a news entry is not required for this change. |
Member
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Wow, great catch!
Could you please add a comment(review below) so that we know why this is needed?
Member
|
The failing Windows test is likely because of https://github.com/python/cpython/pull/137016/files. Could you please try pulling in the changes from there? |
Contributor
Author
|
Yes, applying the changes from #137016 fixes the Windows crash. |
Member
|
We need this to be merged first https://github.com/python/cpython/pull/137016/files |
Contributor
|
Also, I'm not sure I get why we DECREF Maybe @markshannon can shed a light to this, because of #118459 |
Member
Detach is for removing the executor from a code object. The code object owns a strong reference to the executor so we must decref it. |
Contributor
|
@Fidget-Spinner Thanks! |
Contributor
Author
|
The crashes were caused by a double untrack exposed by the fix.
The fix: Guard the untrack call with |
Contributor
Author
|
@Fidget-Spinner I've added the news entry. |
Member
|
So I'm gonna leave this around for a few more days. However could you please add a code comment above the if that tp_traverse might untrack the onject once it breaks cycles, so we need to not double-untrack it please? |
Contributor
Author
|
Got it, sounds good — I’ll do that. |
picnixz
changed the title
Contributor
Contributor
Author
|
@sergey-miryanov The project doesn't even build right now, so I'm unable to verify it. I'm building Python from the Build Logs
|
Member
|
@ashm-dev does the project build if you apply this patch here to main? |
Contributor
Author
|
@Fidget-Spinner No, it doesn't build even with my patch applied. It looks like the JIT is broken again. |
Member
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
It's not broken (apart from the refcount leak I think), the current patch seems to be breaking it.
Contributor
Author
|
But it doesn't build on main either, even without my patch. I posted the logs above. |
sergey-miryanov
approved these changes
Dec 19, 2025
Contributor
sergey-miryanov
left a comment
sergey-miryanov
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks!
Member
@ashm-dev I think you're misunderstanding me. I'm asking if it builds with this patch currently on this branch? |
Contributor
Author
|
@Fidget-Spinner Yes, it builds! |
Contributor
|
I'm seeing GHA segfaults in x86_64-pc-windows-msvc/msvc (Debug) on this commit and the subsequent one, but not on earlier commits: |
cocolato
pushed a commit
to cocolato/cpython
that referenced
this pull request
Dec 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Uh oh!
There was an error while loading. Please reload this page.