Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Apr 9, 2025

This is done to follow Python's convention. With this change now channels can be used with the aclosing() context manager for example.

The name close() is still available for backwards compatibility, but deprecated.

Fixes #379.

Copilot AI review requested due to automatic review settings April 9, 2025 07:47
@llucax llucax requested a review from a team as a code owner April 9, 2025 07:47
@llucax llucax requested a review from Marenz April 9, 2025 07:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

@github-actions github-actions bot added part:docs Affects the documentation part:channels Affects channels implementation labels Apr 9, 2025
@llucax llucax self-assigned this Apr 9, 2025
@llucax llucax added this to the v1.8.0 milestone Apr 9, 2025
This is done to follow Python's convention. With this change now
channels can be used with the `aclosing()` context manager for example.

The name `close()` is still available for backwards compatibility, but
deprecated.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax enabled auto-merge April 9, 2025 07:54
@shsms
Copy link
Contributor

shsms commented Apr 9, 2025

Wait, haven't we spoken about this many times already? The close method doesn't wait for the channel to drain completely and fully close. It just stops the channel from accepting new messages, and initiates the close process, and so aclose doesn't make sense?

@shsms
Copy link
Contributor

shsms commented Apr 9, 2025

Maybe we change the implementation to wait until the receivers are drained? Alternatively, create a task that would trigger the condition variable, and make close sync.

@llucax
Copy link
Contributor Author

llucax commented Apr 9, 2025

Moving the discussion to the issue (in case we close this PR in favor of another approach, so the discussion is preserved until the issue is fixed):

Copy link
Contributor

@shsms shsms left a comment

Choose a reason for hiding this comment

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

I've changed my opinion on this and think the action we take (of stopping senders) is already sufficient to call this aclose.

@llucax llucax added this pull request to the merge queue Apr 14, 2025
Merged via the queue into frequenz-floss:v1.x.x with commit 0d7c2b7 Apr 14, 2025
5 checks passed
@llucax llucax deleted the chan-aclose branch April 14, 2025 08:49
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a new alias for channels async close() called aclose()

2 participants