Uh oh!
There was an error while loading. Please reload this page.
gh-151524: Avoid using instrumentation callback result after Py_DECREF - #151525
Conversation
chris-eibl
left a comment
There was a problem hiding this comment.
The change lgtm, but I'd create a news entry, like almost all of the fixes for the umbrella issue #146102 did so far.
cc @pablogsal
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
lpyu001
commented
Jun 16, 2026
I’ve submitted the news entry.thanks |
sobolevn
left a comment
There was a problem hiding this comment.
Question: was it ever tested? Or was just the RC of res higher than 1, so no crash happened?
| @@ -0,0 +1,2 @@ | |||
| Avoid comparing the result of a ``sys.monitoring`` callback after | |||
There was a problem hiding this comment.
This would be a user-facing news entry. Users care about crashes (which could happen here), not about RC :)
Let's rephrase it.
ZeroIntensity
left a comment
There was a problem hiding this comment.
This isn't UB. _PyInstrumentation_DISABLE is an immortal object; Py_DECREF operations on it are a no-op.
>>> import sys
>>> sys._is_immortal(sys.monitoring.DISABLE)
TrueThat said, I do agree that it's misleading. Let's either remove the Py_DECREF call entirely and/or add an assertion that it's immortal.
Uh oh!
There was an error while loading. Please reload this page.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
aisk
commented
Jul 21, 2026
Hi @lpyu001 I see you made some update on this PR, and currently I think it look fine. Can you reply "I have made the requested changes; please review again" in the comments as the bot said, to require other reviewers continue the work? |
lpyu001
commented
Jul 22, 2026
I have made the requested changes; please review again |
Thanks for making the requested changes! @chris-eibl, @ZeroIntensity: please review the changes made to this pull request. |
chris-eibl
left a comment
There was a problem hiding this comment.
Lgtm and helps me who didn't realize _PyInstrumentation_DISABLE is immortal 👍
lpyu001
commented
Aug 7, 2026
The PR has been mergeable for two weeks. Could we go ahead and merge it? @ZeroIntensity@chris-eibl |
Uh oh!
There was an error while loading. Please reload this page.
Thanks @lpyu001 for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
Thanks @lpyu001 for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15. |
Thanks @lpyu001 for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
GH-155349 is a backport of this pull request to the 3.14 branch. |
GH-155350 is a backport of this pull request to the 3.15 branch. |
GH-155351 is a backport of this pull request to the 3.13 branch. |
bedevere-bot
commented
Aug 7, 2026
|
bedevere-bot
commented
Aug 7, 2026
|
Uh oh!
There was an error while loading. Please reload this page.