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

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

Open
richardebeling opened this issue May 13, 2024 · 2 comments
Labels
[C] Backend Focuses on backend implementation [P] Minor Minor priority [T] Bug This is a bug. We don't like it. Please get rid of it.

Comments

@richardebeling
Copy link
Member

richardebeling commented May 13, 2024

See discussion in #1361.

Our two weak notification receivers (grep weak=True) were intended to only be active while the current request is being handled. However, we can not assume this, as garbage collection is not guaranteed. We've had at least two sporadic test failures in the last weeks due to this. To replicate, run the tests with a patched manage.py where you added

import gc
gc.disable()

to the top and run the tests with --shuffle=3290553936 (any shuffle where the user_edit view is accessed at least once before the evaluation_edit view test that removes participants and triggers reward point granting.

I don't know yet how to properly implement the behavior we intended to have. If may suffice to unregister the signal receiver at the end of the view.

Edit: While we're at it, I think the grant_reward_points_after_delete function could use a assert not grantings in the if reverse in the if granting case -- we only expect grantings to be set once, otherwise, overwriting of the old value of grantings would be unexpected.

@richardebeling richardebeling added [C] Backend Focuses on backend implementation [T] Bug This is a bug. We don't like it. Please get rid of it. labels May 13, 2024
@Kakadus
Copy link
Collaborator

Kakadus commented Jun 17, 2024

same thing with seed 1116753744 at test_remove_participants_proportional_reward_points here

@Kakadus
Copy link
Collaborator

Kakadus commented Jul 1, 2024

we discarded the approach in #2229 because it seemed implementation specific. Instead, we want a contextmanager which disconnects the signal directly.

@janno42 janno42 added the [P] Minor Minor priority label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C] Backend Focuses on backend implementation [P] Minor Minor priority [T] Bug This is a bug. We don't like it. Please get rid of it.
Development

Successfully merging a pull request may close this issue.

3 participants