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: change chan closed(&mut self) to closed(&self) #2939

Merged
merged 2 commits into from Oct 12, 2020

Conversation

zaharidichev
Copy link
Contributor

Changes UnboundedSender::closed and Sender::closed to not take self as mut

Refs: #2928

@taiki-e taiki-e mentioned this pull request Oct 10, 2020
21 tasks
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Oct 10, 2020
Comment on lines -150 to 153
pub(crate) async fn closed(&mut self) {
pub(crate) async fn closed(&self) {
use std::future::Future;
use std::pin::Pin;
use std::task::Poll;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know enough about the internals of channels to know whether it relies on the exclusivity of a mutable reference. However if so, we do not have any loom tests that would catch it, because such a test would not compile before this change was applied.

I would like to see a loom test that calls send and closed on the same Arc<Sender> from multiple threads concurrently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Darksonn Good point! I will verify that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems these are simply awaiting on a Notify so there should not be much of a problem, but still there is the loom test

Copy link
Member

Choose a reason for hiding this comment

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

I would assume the &mut self requirement (if there was one) would come from the self.closed call but that seems to take &self so this should be fine.

@LucioFranco LucioFranco changed the title sync: change chan closed(&mut self) -> closed(&self) sync: change chan closed(&mut self) to closed(&self) Oct 12, 2020
@LucioFranco LucioFranco merged commit b575082 into tokio-rs:master Oct 12, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants