GH-90699: fix ref counting of static immortal strings

[kumaraditya303]

Without this they can cause negative refcount in _Py_RefTotal.

• Issue: Replace _Py_IDENTIFIER() with statically initialized objects. #90699

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit f530250 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[corona10]

Thanks for the work, Would you like to provide a reproducible way for detecting leaks?

[kumaraditya303]

Thanks for the work, Would you like to provide a reproducible way for detecting leaks?

The following script causes negative ref count:

for i in range(1000):
    a = repr(False)

Output:

@kumaraditya303 ➜ /workspaces/cpython (main ✗) $ ./python -X showrefcount main.py 
[-1001 refs, 0 blocks]

[kumaraditya303]

@corona10: The following script causes negative refcount with TextIOWrapper:

# main.py
from io import TextIOWrapper

a = TextIOWrapper(open('main.py','rb'))
for i in a:
    pass
a.close()

Output:

@kumaraditya303 ➜ /workspaces/cpython (main ✗) $ ./python -X showrefcount main.py 
[-2 refs, 0 blocks]

[corona10]

lgtm

I reproduced the issue with the attached scripts and issues are solved by @kumaraditya303 's patch :)

Thanks for the work!

[corona10]

@kumaraditya303

Sorry Kumar, Looks like leaks exist through this patch.
PTAL

With patch

beginning 6 repetitions
123456
......
test_io leaked [2, 2, 2] references, sum=6
test_io failed (reference leak) in 3 min 23 sec

== Tests result: FAILURE ==

1 test failed:
    test_io

Total duration: 3 min 23 sec
Tests result: FAILURE

Without Patch

Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 2.90 Run tests sequentially
0:00:00 load avg: 2.90 [1/1] test_io
beginning 6 repetitions
123456
......
test_io passed in 3 min 23 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 3 min 23 sec
Tests result: SUCCESS

[bedevere-bot]

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[kumaraditya303]

Looking, I wonder why only two bots failed if there are refleaks.

[kumaraditya303]

It is also leaking at my local environment :(

Yeah, I am also able to reproduce, bisecting which one is causing leak

[kumaraditya303]

I am getting refleak on main branch too, test.test_io.CTextIOWrapperTest.test_reconfigure_locale is failing.

[kumaraditya303]

Refleak was fixed by #94858

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 0a76abd 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[kumaraditya303]

Refleak buildbots are failing on main too, see #94979

[corona10]

LGTM
Thank you, Kumar!

[miss-islington]

Thanks @kumaraditya303 for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-95037 is a backport of this pull request to the 3.11 branch.

[kumaraditya303]

Thanks for the review @corona10!