Skip to content

Ensure deleted receivers get cleaned up#45

Merged
sahas-subramanian-frequenz merged 2 commits intofrequenz-floss:v0.x.xfrom
shsms:cleanup-dropped-receivers
Nov 17, 2022
Merged

Ensure deleted receivers get cleaned up#45
sahas-subramanian-frequenz merged 2 commits intofrequenz-floss:v0.x.xfrom
shsms:cleanup-dropped-receivers

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented Nov 11, 2022

Broadcast receivers were not getting cleaned up when then go out of scope, because Broadcast instances were holding on to a strong reference to them. And because these receivers were not being read from anymore, but were still getting messages, their buffers would overflow, and the logs would get flooded with warnings.

This PR fixes this issue by storing just weak references to the receivers in the Broadcast instances.

@github-actions github-actions bot added part:channels Affects channels implementation part:tests Affects the unit, integration and performance (benchmarks) tests labels Nov 11, 2022
@shsms shsms marked this pull request as ready for review November 17, 2022 11:15
@shsms shsms requested a review from a team as a code owner November 17, 2022 11:15
... by storing just a weakref to a receiver in the channel object.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms shsms force-pushed the cleanup-dropped-receivers branch from 49883f3 to 3dc9ba3 Compare November 17, 2022 11:17
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would add a note on the RELEASE_NOTES as it seems like an important bug fix, and also I would add a better explanation of what kind of problems we observed before this fix in the PR description (which will be used as the merge commit message).

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Copy link
Contributor Author

shsms commented Nov 17, 2022

LGTM, but I would add a note on the RELEASE_NOTES as it seems like an important bug fix, and also I would add a better explanation of what kind of problems we observed before this fix in the PR description (which will be used as the merge commit message).

Both are done.

@sahas-subramanian-frequenz sahas-subramanian-frequenz merged commit 4cceb39 into frequenz-floss:v0.x.x Nov 17, 2022
@shsms shsms deleted the cleanup-dropped-receivers branch November 17, 2022 16:01
@leandro-lucarella-frequenz
Copy link
Contributor

Cheater! 😆

image

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

Labels

part:channels Affects channels implementation part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Receivers are not getting deleted when they go out of scope

3 participants