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

Fix a few bugs related to GrpcStreamBroadcaster #42

Merged
merged 2 commits into from
May 15, 2024

Conversation

llucax
Copy link
Contributor

@llucax llucax commented May 15, 2024

When a broadcaster already existed, a new one was created but immediately discarded, as it was never saved. Instead we really want to avoid creating a new broadcaster if one already exists.

Also when introducing the GrpcStreamBroadcaster from client-base we forgot to forward the selected retry strategy to it.

When a broadcaster already existed, a new one was created but
immediately discarded, as it was never saved. Instead we really want to
avoid creating a new broadcaster if one already exists.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax requested review from a team as code owners May 15, 2024 07:36
@github-actions github-actions bot added part:docs Affects the documentation part:client Affects the client code labels May 15, 2024
When introducing the `GrpcStreamBroadcaster` from `client-base` we
forgot to forward the selected retry strategy.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax self-assigned this May 15, 2024
@llucax llucax changed the title Fix leakage of GrpcStreamBroadcaster instances Fix a few bugs related to GrpcStreamBroadcaster May 15, 2024
@llucax llucax added this to the v0.4.0 milestone May 15, 2024
@llucax llucax enabled auto-merge May 15, 2024 07:44
@llucax
Copy link
Contributor Author

llucax commented May 15, 2024

Enabled auto-merge.

@llucax llucax added the type:bug Something isn't working label May 15, 2024
Copy link

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

dict.setdefault() is sneakily hiding the leakage, good catch! LGTM

@llucax llucax added this pull request to the merge queue May 15, 2024
@llucax
Copy link
Contributor Author

llucax commented May 15, 2024

dict.setdefault() is sneakily hiding the leakage, good catch! LGTM

Yeah, I guess that's one disadvantage of using dict.setdefault(), it is not super obvious what's going on.

Merged via the queue into frequenz-floss:v0.x.x with commit 67de281 May 15, 2024
15 checks passed
@llucax llucax deleted the fix-leak branch May 15, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:client Affects the client code part:docs Affects the documentation type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants