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

Systematically replace __del__ with weakref.finalize() #246

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Nov 15, 2024

Closes #141

See #141 (comment) for the rationale.

Note that Event and Stream are a bit special, because the regular __init__()s raise. The pattern adopted for both is to add _enable_finalize(), which NEEDS to be called from derived classes; that is a potential trap. Should we try to avoid that trap?

@rwgk rwgk requested a review from leofang November 15, 2024 19:45
@leofang leofang added enhancement Any code-related improvements P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Nov 16, 2024
@leofang leofang added this to the cuda.core beta 2 milestone Nov 16, 2024
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Thanks, Ralf! A general comment: We can't use self.close as the finalizer because it'd still hold a reference to self (see @wence-'s original #87 (comment)). The most simple thing we could do is to move it out of the class, so instead of

class Something:

    def __init__(self, ...):
        self._handle = ...
        weakref.finalize(self, self.close)

    def close(self, ...):
        if self._handle:
            # do something

we do

# note the 1st arg name is still "self", to mimic a method
def _close_something(self, ...):
    if self._handle:
        # do something

class Something:

    def __init__(self, ...):
        self._handle = ...
        weakref.finalize(self, _close_something)

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 16, 2024

Ah, confirmed, by looking more. Yesterday I was jumping to a wrong conclusion, twisty explanation omitted. I'll try again.

@rwgk
Copy link
Collaborator Author

rwgk commented Nov 16, 2024

I think I found a nice pattern to avoid reference cycles. This is my updated toy example:

https://github.com/rwgk/stuff/blob/7d93794069898f245e14fb94d779816291fde1ff/random_attic/weakref_finalize_toy_example.py

The trick is to introduce a level of indirection, self._members, and tie weakref.finalize() to those.

I still need to adopt that pattern in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module enhancement Any code-related improvements P0 High priority - Must do!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to make class destruction more robust
2 participants