7900: sync: add closed method to broadcast::WeakSender#62
7900: sync: add closed method to broadcast::WeakSender#62martin-augment wants to merge 1 commit intomasterfrom
closed method to broadcast::WeakSender#62Conversation
Summary of ChangesHello @martin-augment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis change refactors the broadcast channel closure semantics in Tokio's sync module. The Tail struct's ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review - PR #62: sync: add
|
🤖 Augment PR SummarySummary: Add
🤖 Was this summary useful? React with 👍 or 👎 |
|
|
||
| /// True if the channel is closed. | ||
| closed: bool, | ||
| /// True if there are no senders. |
There was a problem hiding this comment.
The doc comment for has_senders appears inverted: close_channel() sets has_senders = false when the last Sender drops, and receivers treat !has_senders as closed. Consider updating the comment to reflect that true means there are senders.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Augment AI reviewer is correct! The docstring for has_senders is still related to the old/removed closed flag. It should be updated to say that it would be true if there are live strong senders instead.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tokio/src/sync/broadcast.rs (2)
1037-1057:closed_for_sendersfollows the correct Notify pattern — but consider the semantic gap withSender::closed().The implementation correctly creates the
notifiedfuture before checkingrx_cntunder the lock, which avoids missing notifications. The cooperative wrapper and trace leaf are consistent with similar methods in the codebase (e.g.,watch::Sender::closed()).One observation: this method name (
closed_for_senders) and its semantics (waits until all receivers drop) are shared by bothSender::closed()andWeakSender::closed(). ForSender::closed(), this works because a strong sender keeps the channel alive — it's waiting for all receivers to leave. ForWeakSender::closed(), however, the channel may already be closed (all strong senders dropped), yet this method still waits for receivers to drain. That's the intended design per the tests, but the doc comment onWeakSender::closed()should clarify that it waits for receivers to drop rather than for the channel to close (which may have already happened).
1107-1136: Doc comment says "Receivers subscribed to this channel" but does not clarify the WeakSender-specific behavior.The doc mirrors
Sender::closed()("completes when the number of Receivers reaches zero"), but for aWeakSenderthe channel may already have no strong senders — meaning the semantic context is different. A user might expectWeakSender::closed()to complete immediately once all strong senders are gone (channel is "closed"), but instead it waits for receivers to drain. Consider adding a note like: "This completes when all receivers have been dropped, regardless of whether strong senders still exist."The example is good and clearly demonstrates the expected behavior with
now_or_never().
There was a problem hiding this comment.
Code Review
The pull request successfully adds the closed method to broadcast::WeakSender and refactors the internal state management to decouple sender-based closure from receiver counts. The changes are well-tested and maintain consistency with existing patterns in the broadcast module. I have identified one minor documentation error in the Tail struct where a comment incorrectly describes the state of a boolean field.
|
|
||
| /// True if the channel is closed. | ||
| closed: bool, | ||
| /// True if there are no senders. |
There was a problem hiding this comment.
The comment for has_senders is incorrect. Based on the implementation in Sender::new_with_receiver_count (where it is initialized to true) and close_channel (where it is set to false when senders are dropped), this field is true when there are active senders, not when there are none.
| /// True if there are no senders. | |
| /// True if there are senders. |
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! The docstring for has_senders is still related to the old/removed closed flag. It should be updated to say that it would be true if there are live strong senders instead.
value:good-to-have; category:documentation; feedback: The Claude AI reviewer is correct! The docstring for has_senders is still related to the old/removed closed flag. It should be updated to say that it would be true if there are live strong senders instead. |
7900: To review by AI