Fix Ref Leak

[Vizonex]

What do these changes do?

Removing the ref counter will prevent future ref-leaking Honestly this addition was not my idea and should've never passed due to the fact that cython has it's own ref-counter-system and should be only used in the case of passing a python object to <void*> and back out again

Are there changes in behavior for the user?

Ref counters fixed means we don't end up having ref leaks ever again

Related issue number

fixes #159

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes

[bdraco]

cached_property still has the inc as well.. I thought we had a failure in production without this. I'll retest

[Vizonex]

cached_property still has the inc as well.. I thought we had a failure in production without this. I'll retest

Sounds good. :) I just added a fix for it as well.

[bdraco]

downstream ci passes. So Py_INCREF is unneeded. Sorry about asking about it in the original pr leading down the wrong path.

[bdraco]

Looks like Cython is taking care of the refcounting for us on the borrowed ref so we should be all good with this

[bdraco]

Thanks @Vizonex

[bdraco]

I'll do a release as soon as the CI passes

[Vizonex]

I'll do a release as soon as the CI passes

Your welcome :) I should've been a lot more defensive when I was making the original pr at least it's safe for us to use now.

[Vizonex]

Hopefully the next release fixes the bug.

[bdraco]

https://docs.python.org/3/c-api/call.html#c.PyObject_CallOneArg -- returns a new ref
https://docs.python.org/3/c-api/dict.html#c.PyDict_GetItem -- returns a borrowed ref

Hmm do we still have a leak if the value comes from PyObject_CallOneArg?

[Vizonex]

downstream ci passes. So Py_INCREF is unneeded. Sorry about asking about it in the original pr leading down the wrong path.

It's all good. We all learn from our mistakes.

[Vizonex]

https://docs.python.org/3/c-api/call.html#c.PyObject_CallOneArg -- returns a new ref https://docs.python.org/3/c-api/dict.html#c.PyDict_GetItem -- returns a borrowed ref

Hmm do we still have a leak if the value comes from PyObject_CallOneArg?

Might be something worth testing do you know of a pytest we could add to prevent future bogus claims?

[bdraco]

Check this comment #159 (comment) -- It looks like they are concerned about the same issue

[bdraco]

PyObject_CallOneArg returns new ref → refcount = 1 (we own it)
Py_DECREF drops our ownership → refcount = 0 -- likely freed

... I think we want...

PyObject_CallOneArg returns new ref → refcount = 1 (we own it)
PyDict_SetItem stores it and increments → refcount = 2 (dict owns 1, we own 1)
Py_DECREF drops our ownership → refcount = 1 (dict owns it)
return val INCREFs for return → refcount = 2 (dict owns 1, caller owns 1)

I think the Py_DECREF should happen after the PyDict_SetItem

[bdraco]

Looks good now

[bdraco]

docs.python.org/3/c-api/call.html#c.PyObject_CallOneArg -- returns a new ref docs.python.org/3/c-api/dict.html#c.PyDict_GetItem -- returns a borrowed ref
Hmm do we still have a leak if the value comes from PyObject_CallOneArg?

Might be something worth testing do you know of a pytest we could add to prevent future bogus claims?

I added tests

[codspeed-hq]

CodSpeed Performance Report

Merging #162 will not alter performance

_{Comparing Vizonex:fix-ref-leak (3b20806) with master (21ef369)}

Summary

✅ 4 untouched

[codecov]

Codecov Report

❌ Patch coverage is 96.47059% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.63%. Comparing base (21ef369) to head (3b20806).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
tests/test_under_cached_property.py	95.45%	2 Missing ⚠️
tests/test_cached_property.py	97.56%	1 Missing ⚠️

❌ Your project status has failed because the head coverage (97.35%) is below the target coverage (98.20%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   97.76%   97.63%   -0.14%     
==========================================
  Files          17       17              
  Lines         762      847      +85     
  Branches       41       44       +3     
==========================================
+ Hits          745      827      +82     
- Misses          9       12       +3     
  Partials        8        8              

Flag	Coverage Δ
CI-GHA	97.63% <96.47%> (-0.14%)	⬇️
MyPy	91.84% <78.82%> (-1.48%)	⬇️
OS-Linux	97.48% <100.00%> (+0.56%)	⬆️
OS-Windows	94.52% <100.00%> (+1.24%)	⬆️
OS-macOS	94.52% <100.00%> (+1.24%)	⬆️
Py-3.10.11	92.69% <100.00%> (+1.65%)	⬆️
Py-3.10.18	94.52% <100.00%> (+1.24%)	⬆️
Py-3.11.13	96.11% <100.00%> (+0.88%)	⬆️
Py-3.11.9	94.29% <100.00%> (+1.29%)	⬆️
Py-3.12.10	94.29% <100.00%> (+1.29%)	⬆️
Py-3.12.11	96.11% <100.00%> (+0.88%)	⬆️
Py-3.13.5	94.29% <100.00%> (+1.29%)	⬆️
Py-3.13.7	96.11% <100.00%> (+0.88%)	⬆️
Py-3.13.8t	96.11% <100.00%> (+0.88%)	⬆️
Py-3.14.0	96.08% <100.00%> (+0.89%)	⬆️
Py-3.14.0t	96.08% <100.00%> (+0.89%)	⬆️
Py-3.9.13	92.67% <100.00%> (+1.66%)	⬆️
Py-3.9.23	94.50% <100.00%> (+1.24%)	⬆️
Py-pypy3.10.16-7.3.19	78.08% <12.34%> (-14.92%)	⬇️
Py-pypy3.9.19-7.3.16	78.03% <12.34%> (-14.95%)	⬇️
VM-macos-latest	94.52% <100.00%> (+1.24%)	⬆️
VM-ubuntu-latest	97.48% <100.00%> (+0.56%)	⬆️
VM-windows-11-arm	94.29% <100.00%> (+1.29%)	⬆️
VM-windows-latest	94.52% <100.00%> (+1.24%)	⬆️
pytest	97.48% <100.00%> (+0.56%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bdraco]

codecov reporting is wrong. Its reporting on the genexpr inner code which is obviously covered