Skip to content

test_intern in test_sys leaks negative references in free-threaded build #122420

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

Closed
colesbury opened this issue Jul 29, 2024 · 2 comments
Closed
Labels
3.13 bugs and security fixes 3.14 bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Jul 29, 2024

Bug report

The test_intern test "leaks" -2 references in the free-threaded build. This isn't caught by the refleak build bots because negative refleaks are treated as "suspicious" rather than "failing". This is a problem because the negative "leak" can hide a real leak somewhere else.

I think there's likely something wrong with the accounting for references in the immortalize=1 case in intern_common because this shows up in the free-threaded build, but not the default build.

./python -m test test_sys -m test_intern -R 3:3

Using random seed: 3390398091
0:00:00 load avg: 14.98 Run 1 test sequentially
0:00:00 load avg: 14.98 [1/1] test_sys
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
X8. ...
test_sys leaked [-2, -2, -2] references, sum=-6 (this is fine)

Linked PRs

@colesbury colesbury added type-bug An unexpected behavior, bug, or error 3.13 bugs and security fixes topic-free-threading 3.14 bugs and security fixes labels Jul 29, 2024
@colesbury
Copy link
Contributor Author

cc @encukou

@colesbury
Copy link
Contributor Author

I think this is just an issue with refleak.py:

interned_immortal_after = getunicodeinternedsize(
# Use an internal-only keyword argument that mypy doesn't know yet
_only_immortal=True) # type: ignore[call-arg]
alloc_after = getallocatedblocks() - interned_immortal_after
rc_after = gettotalrefcount() - interned_immortal_after * 2

We now account for those two references in intern_common, so we shouldn't subtract them out in refleak.py:

cpython/Objects/unicodeobject.c

Lines 15518 to 15527 in 046670c

if (!_Py_IsImmortal(s)) {
/* The two references in interned dict (key and value) are not counted.
unicode_dealloc() and _PyUnicode_ClearInterned() take care of this. */
Py_SET_REFCNT(s, Py_REFCNT(s) - 2);
#ifdef Py_REF_DEBUG
/* let's be pedantic with the ref total */
_Py_DecRefTotal(_PyThreadState_GET());
_Py_DecRefTotal(_PyThreadState_GET());
#endif
}

colesbury added a commit to colesbury/cpython that referenced this issue Jul 29, 2024
…leak.py

The `_PyUnicode_Intern*` functions already adjust the total refcount, so
we don't want to readjust it in refleak.py.
encukou pushed a commit that referenced this issue Jul 29, 2024
…GH-122421)

The `_PyUnicode_Intern*` functions already adjust the total refcount, so
we don't want to readjust it in refleak.py.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 29, 2024
…eak.py (pythonGH-122421)

The `_PyUnicode_Intern*` functions already adjust the total refcount, so
we don't want to readjust it in refleak.py.
(cherry picked from commit ac8da34)

Co-authored-by: Sam Gross <[email protected]>
encukou pushed a commit that referenced this issue Jul 30, 2024
…leak.py (GH-122421) (GH-122430)

The `_PyUnicode_Intern*` functions already adjust the total refcount, so
we don't want to readjust it in refleak.py.
(cherry picked from commit ac8da34)

Co-authored-by: Sam Gross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes 3.14 bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

1 participant