GH-115776: Static object are immortal, so mark them as such.#117673
GH-115776: Static object are immortal, so mark them as such.#117673markshannon merged 3 commits into
Conversation
|
Should this be backported to 3.12? Is it an actual bug / would we expect any impact on third-party extensions because of this? |
Sorry, something went wrong.
|
@eduardo-elizondo do you recall why the refcount was set to 1 for extension code? |
Sorry, something went wrong.
|
Fixes the issue with the latest CPython master. There's a comment above the definitions that also needs adapting. |
Sorry, something went wrong.
ericsnowcurrently
left a comment
There was a problem hiding this comment.
This seems reasonable. An initial refcount of 1 makes the object effectively immortal without any of the benefits.
The only potential problem is if an extension is expecting their static type/object to have a normal refcount (starting at 1). However, that probably isn't worth worrying about.
The only thing I'd recommend is that you update the comment at line 118 to reflect the change. If you know of any other similar comments elsewhere in the code it would be worth updating those too.
Sorry, something went wrong.
My gut tells me a changelog entry should suffice there. Extension authors (especially ones who care about this detail) are used to this sort of thing. |
Sorry, something went wrong.
|
@markshannon This change is totally valid - the reason it was not done in the first place is to avoid incompatibility with extension code. There could be asserts/tests on exact refcount values and this change would have broken those builds/tests. You can even see it in the immoral objects PR where I had to change _testembed.c to modify the There's an argument on whether or not having any exact refcounts checks are correct, but I didn't want to challenge it at the time. Thus, to avoid any problems with extension code and reduce the surface area of impact, I localized it to just affect the CPython build. That said, happy to see that you are now cleaning this up! |
Sorry, something went wrong.
|
For what it's worth, this change did break one numpy test, see the PR linked above. That said, I think this is a good change and it was trivial to update the test by adding a python version check. |
Sorry, something went wrong.
edited by bedevere-app
Bot
LoadingUh oh!
There was an error while loading. Please reload this page.
Copy link Copy MarkdownSorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.