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

provide a non-destructive mechanism to determine if a sink/stream are paired #2797

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Conversation

tones111
Copy link
Contributor

@tones111 tones111 commented Nov 7, 2023

The only mechanism I could find to determine if a sink and stream are paired is reunite which takes ownership of the values. The mechanism proposed here allows the caller to determine if the values are paired without mutation.

This behavior is useful in the case where a service receives messages from multiple connections and would like to send messages to the non-originating sinks. Currently there appears to be no way for the server to determine which sink is paired with the connection that originated the message.

I'm not tied to the function names and welcome any suggestions.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks, this seems reasonable. Channels also have similar APIs.

@@ -69,6 +69,11 @@ impl<T> BiLock<T> {
(Self { arc: arc.clone() }, Self { arc })
}

/// Returns `true` only if the two `BiLock<T>`s originated from the same call to `BiLock::new`.
pub fn are_paired(first: &Self, second: &Self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this different than others? (are_paired(_, _) vs is_pair(&self, _)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no technical reason. I went with the non-method implementation for BiLock because I felt like the instances are more independent than the sink/stream pair, but the primary purpose of the abstraction is to only be coupled to one other instance so the method syntax is still reasonable. The other benefit of using the method signature is that it's more consistent with the sink/stream api, which would be more consistent with reunite.

I've made that change and rebased to hopefully get a clean CI run. Thanks for taking a look.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks.

I noticed a similar function in tokio called is_pair_of. It would make sense to give it the same name to make it easier for users to find.

Also, we have split in io module too. Could you add the same methods to it?

}

#[test]
#[cfg_attr(docsrs, doc(cfg(feature = "sink")))]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg_attr(docsrs, doc(cfg(feature = "sink")))]

Test code will not be displayed in docs.

use crate::{sink::Sink, stream::StreamExt};
use core::marker::PhantomData;

struct NopStream<Item> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This struct is a hack because I couldn't find a type to use without introducing a dependency. I'm open to suggestions if there's something available that I'm missing.

@taiki-e taiki-e added A-stream Area: futures::stream A-sink Area: futures::sink A-io Area: futures::io A-lock Area: futures::lock labels Nov 13, 2023
@taiki-e taiki-e merged commit d3962c1 into rust-lang:master Nov 13, 2023
25 checks passed
@taiki-e taiki-e added the 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. label Nov 13, 2023
@taiki-e taiki-e mentioned this pull request Dec 24, 2023
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Dec 24, 2023
@taiki-e
Copy link
Member

taiki-e commented Dec 24, 2023

Published in 0.3.30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.3-backport: completed A-io Area: futures::io A-lock Area: futures::lock A-sink Area: futures::sink A-stream Area: futures::stream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants