Skip to content

GH-115776: Static object are immortal, so mark them as such. #117673

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

Merged
merged 3 commits into from
Apr 16, 2024

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Apr 9, 2024

@mdboom
Copy link
Contributor

mdboom commented Apr 9, 2024

Should this be backported to 3.12? Is it an actual bug / would we expect any impact on third-party extensions because of this?

@markshannon
Copy link
Member Author

It only surfaced as a result of #116115, so maybe not.

@markshannon
Copy link
Member Author

@eduardo-elizondo do you recall why the refcount was set to 1 for extension code?

@scoder
Copy link
Contributor

scoder commented Apr 9, 2024

Fixes the issue with the latest CPython master.

There's a comment above the definitions that also needs adapting.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mdboom
Copy link
Contributor

mdboom commented Apr 11, 2024

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.

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.

@markshannon markshannon merged commit c053d52 into python:main Apr 16, 2024
36 checks passed
@eduardo-elizondo
Copy link
Contributor

@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 Py_REFCNT(str1) == 1 check to _Py_IsImmortal(str1)

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!

@ngoldbaum
Copy link
Contributor

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.

@markshannon markshannon deleted the static-is-immortal branch August 6, 2024 10:16
@puremourning
Copy link

It broke vim tests too vim/vim#15838 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants