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

manually del weakref functions to make sure they are disconnected #2229

Closed
wants to merge 1 commit into from

Conversation

Kakadus
Copy link
Collaborator

@Kakadus Kakadus commented Jun 17, 2024

fix #2189

tested by reverting commit, adding gc.disable() and testing with --shuffle=1116753744

@@ -1323,6 +1323,7 @@ def notify_reward_points(grantings, **_kwargs):
"editable": editable,
"questionnaires_with_answers_per_contributor": questionnaires_with_answers_per_contributor,
}
del notify_reward_points # cleanup receiver
Copy link
Member

Choose a reason for hiding this comment

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

Why does this solve the problem? From the docs:

Deletion of a name removes the binding of that name from the local or global namespace, depending on whether the name occurs in a global statement in the same code block.

To me, this sounds like del x and x going out of scope are the same thing. Both don't ensure that garbage collection actually runs. The tests pass, but that's probably just a CPython implementation detail?

There is Signal.disconnect. Ideally I'd want to use that through a context manager. I can't find any matching context manager in the django docs, we'd probably have to make our own, but that should be feasible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my explaination: python uses both gc and ref counting. Maybe going out of scope does not mean directly that the refcount is decreased.
I tested this entirely without gc and test failures were avoidee

Copy link
Member

Choose a reason for hiding this comment

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

... but this sounds like we would just set ourselves up for the next issue when gc (again) behaves different than we expect. If it is not too much effort, I also think that the explicit disconnect through a context manager would be better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

according to this stackoverflow you are right: refcounting is part of "gc" and is even implementation dependent 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failure due to weak signal receivers having no guarantee of being garbage collected in time
3 participants