Skip to content
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

Remove some over-enthusiastic memory retention. #2482

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

freakboy3742
Copy link
Member

In the process of exploring beeware/rubicon-objc#256, I found a couple of over-enthusiastic memory retentions. These will be causing memory leaks over time.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@mhsmith
Copy link
Member

mhsmith commented Apr 13, 2024

If we remove the retain from the retain/autorelease pair, how do we "Prevent tcv from being deallocated prematurely", as the comment says? My understanding was that the Python object would be deleted at some point during the return process, and because the object was created with alloc, this would reduce the Objective-C object's reference count to zero before it's returned to the caller.

If that understanding is correct, then the retain/autorelease pairs should stay as they are, and the only bug is that the detailedlist should have an autorelease added.

@freakboy3742
Copy link
Member Author

If we remove the retain from the retain/autorelease pair, how do we "Prevent tcv from being deallocated prematurely", as the comment says? My understanding was that the Python object would be deleted at some point during the return process, and because the object was created with alloc, this would reduce the Objective-C object's reference count to zero before it's returned to the caller.

tcv is being created with an alloc method, so it's implicitly already owned; there's no need to explicitly retain. The autorelease is what defers the deallocation allowing the calling method to use the object before the deallocation occurs.

As Rubicon currently stands, the autorelease disables the "release on Python garbage collection" behavior, so the reference will live long enough to be handed back to the calling method.

If that understanding is correct, then the retain/autorelease pairs should stay as they are, and the only bug is that the detailedlist should have an autorelease added.

Are you referring to the TogaData instance in detailedlist.py? In that case, the alloc means the object is implicitly retained; the purpose of that object is to be a Python object that we can attach arbitrary values to - so the lifecycle of that object is entirely Python-side. A reference is stored Python-side as part of the underlying data store; when that object is disposed of, there's no longer any need to have a TogaData object, so the Python wrapper will be garbage collected, and the ObjC instance can be disposed. There's no need to autorelease because ObjC will never have an owned reference to this object; it's only ever returned as a data value for ObjC to read and render.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

As Rubicon currently stands, the autorelease disables the "release on Python garbage collection" behavior, so the reference will live long enough to be handed back to the calling method.

OK, that's the part I was missing. I see it's explained in the docstrings of release and autorelease, but those aren't included in the RTD docs for some reason (beeware/rubicon-objc#48).

@mhsmith mhsmith merged commit 4398e2a into beeware:main Apr 16, 2024
34 checks passed
@freakboy3742 freakboy3742 deleted the memory-retention branch April 16, 2024 22:53
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.

None yet

2 participants