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

sync::broadcast: create Sender::new #5824

Merged
merged 2 commits into from
Jul 15, 2023

Conversation

marcospb19
Copy link
Contributor

@marcospb19 marcospb19 commented Jun 26, 2023

Closes #4958

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Jun 26, 2023
@marcospb19
Copy link
Contributor Author

I don't know how to fix the CI problem:

---- broadcast_channel_panic_caller stdout ----
thread 'broadcast_channel_panic_caller' panicked at 'assertion failed: `(left == right)`
  left: `"/home/runner/work/tokio/tokio/tokio/src/sync/broadcast.rs"`,
 right: `"tokio/tests/sync_panic.rs"`', tokio/tests/sync_panic.rs:22:5

tokio/src/sync/broadcast.rs Outdated Show resolved Hide resolved
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jun 27, 2023
@marcospb19 marcospb19 force-pushed the create-constructor-sender-new branch from 374bb1b to eed672b Compare June 27, 2023 20:58
@marcospb19 marcospb19 force-pushed the create-constructor-sender-new branch from eed672b to 16cea8a Compare July 5, 2023 20:13
@marcospb19 marcospb19 force-pushed the create-constructor-sender-new branch from 16cea8a to 3b486b8 Compare July 5, 2023 20:38
@marcospb19 marcospb19 force-pushed the create-constructor-sender-new branch from 3b486b8 to f5d2b5a Compare July 5, 2023 20:46
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

The new_with_receiver_count method is fine, but I have some comments.

});

pub fn channel<T: Clone>(capacity: usize) -> (Sender<T>, Receiver<T>) {
let tx = unsafe { Sender::new_with_receiver_count(1, capacity) };
Copy link
Contributor

Choose a reason for hiding this comment

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

The two calls to this method should have a SAFETY comment that explains why the code is correct.

Copy link
Contributor Author

@marcospb19 marcospb19 Jul 14, 2023

Choose a reason for hiding this comment

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

    // SAFETY: In the line below we are creating one extra receiver, so there will be 1 in total.

and

    // SAFETY: We don't create extra receivers, so there are 0.

Comment on lines +474 to +488
#[track_caller]
unsafe fn new_with_receiver_count(receiver_count: usize, mut capacity: usize) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an unsafe method, it should have a comment that explains what the safety requirements are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+    /// Creates the sending-half of the [`broadcast`] channel, and provide the receiver count.
+    ///
+    /// See the documentation of [`broadcast::channel`] for more errors when calling this
+    /// function.
+    ///
+    /// # Safety:
+    ///
+    /// The caller must ensure that the amount of receivers for this Sender is correct before
+    /// the channel functionalities are used, the count is zero by default, as this function
+    /// does not create any receivers by itself.


#[test]
fn receiver_count_on_sender_constructor() {
let count_of = |sender: &Sender<i32>| sender.shared.tail.lock().rx_cnt;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to a real method since you're defining it twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I missed that Sender::receiver_count is 100% equivalent to what I was doing.

Updated.

@marcospb19 marcospb19 force-pushed the create-constructor-sender-new branch 2 times, most recently from 67ab20a to d769972 Compare July 14, 2023 17:10
@marcospb19 marcospb19 force-pushed the create-constructor-sender-new branch from d769972 to 4b8d1cd Compare July 14, 2023 17:14
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-sync Module: tokio/sync R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide broadcast::Sender::new and watch::Sender::new functions
3 participants