-
Notifications
You must be signed in to change notification settings - Fork 41
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
added Receiver::recv_owned
#83
base: master
Are you sure you want to change the base?
Conversation
Added `Receiver::recv_owned` as well as supporting types `RecvOwned` and `RecvOwnedInner`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this strategy could also be useful for Sender
as well?
@@ -580,6 +580,19 @@ impl<T> Receiver<T> { | |||
}) | |||
} | |||
|
|||
/// Receives a message from the channel. | |||
/// | |||
/// Like [`Receiver::recv`], but uses a cloned `Receiver`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Like [`Receiver::recv`], but uses a cloned `Receiver`. | |
/// This function is similar to [`Receiver::recv`], but it uses a cloned `Receiver`. |
Avoid using sentence fragments in documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it could be useful for send.
I'll clean up my doc comments.
pub fn recv_owned(&self) -> RecvOwned<T> { | ||
RecvOwned::_new(RecvOwnedInner { | ||
receiver: self.clone(), | ||
listener: None, | ||
_pin: PhantomPinned, | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an example for this function, preferably one that points out where it could be useful.
easy_wrapper! { | ||
/// A future returned by [`Receiver::recv_owned()`]. | ||
#[derive(Debug)] | ||
#[must_use = "futures do nothing unless you `.await` or poll them"] | ||
pub struct RecvOwned<T>(RecvOwnedInner<T> => Result<T, RecvError>); | ||
#[cfg(all(feature = "std", not(target_family = "wasm")))] | ||
pub(crate) wait(); | ||
} | ||
|
||
pin_project! { | ||
#[derive(Debug)] | ||
#[project(!Unpin)] | ||
struct RecvOwnedInner<T> { | ||
// Reference to the receiver. | ||
receiver: Receiver<T>, | ||
|
||
// Listener waiting on the channel. | ||
listener: Option<EventListener>, | ||
|
||
// Keeping this type `!Unpin` enables future optimizations. | ||
#[pin] | ||
_pin: PhantomPinned | ||
} | ||
} | ||
|
||
impl<T> EventListenerFuture for RecvOwnedInner<T> { | ||
type Output = Result<T, RecvError>; | ||
|
||
/// Run this future with the given `Strategy`. | ||
fn poll_with_strategy<'x, S: Strategy<'x>>( | ||
self: Pin<&mut Self>, | ||
strategy: &mut S, | ||
cx: &mut S::Context, | ||
) -> Poll<Result<T, RecvError>> { | ||
let this = self.project(); | ||
|
||
loop { | ||
// Attempt to receive a message. | ||
match this.receiver.try_recv() { | ||
Ok(msg) => return Poll::Ready(Ok(msg)), | ||
Err(TryRecvError::Closed) => return Poll::Ready(Err(RecvError)), | ||
Err(TryRecvError::Empty) => {} | ||
} | ||
|
||
// Receiving failed - now start listening for notifications or wait for one. | ||
if this.listener.is_some() { | ||
// Poll using the given strategy | ||
ready!(S::poll(strategy, &mut *this.listener, cx)); | ||
} else { | ||
*this.listener = Some(this.receiver.channel.recv_ops.listen()); | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than duplicating all of this code from Recv
, why not extract it into a common type? For instance, you could do this:
struct RecvInner<T, Channel: Borrow<Receiver<T>> { ... }
...then do this:
pub struct Recv<'a, T>(RecvInner<T, &'a Receiver<T>>);
pub struct RecvOwned<T>(RecvInner<T, Receiver<T>>);
This would prevent duplicating around 250 lines of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm playing around with this suggestion, but the compiler complains about T
being unused.
I'll figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use PhantomData
to ensure that T
is captured
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose 01fbd7d (it should be possible to fast-forward to that commit if no further adjustments to it are necessary)
Is it worth looking at some benchmarks to see if the referenced version is even necessary? Would the performance hit for using a cloned receiver be that bad? |
No matter what an atomic operation is going to be slower than one that doesn't use atomics. Still, benchmarks would be nice |
Added
Receiver::recv_owned
as well as supporting typesRecvOwned
andRecvOwnedInner
.