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

expose try_recv and try_send on channels #585

Merged
merged 5 commits into from
Mar 16, 2020
Merged

expose try_recv and try_send on channels #585

merged 5 commits into from
Mar 16, 2020

Conversation

yoshuawuyts
Copy link
Contributor

@yoshuawuyts yoshuawuyts commented Nov 25, 2019

Exposes Receiver::try_recv and Sender::try_send on channels. Closes #579. I'm proposing we put the types in the crate's top-level because we're still undecided. I'd like to move to unblock people first, and then consider how we might want to move types around. Thanks!

Screenshot

Screenshot_2019-11-26 async_std sync - Rust

@yoshuawuyts yoshuawuyts requested a review from a user November 25, 2019 18:43
Copy link
Member

@k-nasa k-nasa left a comment

Choose a reason for hiding this comment

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

LGTM!!

@ghost
Copy link

ghost commented Nov 27, 2019

How do you feel about this mild inconsistency where we sometimes use Option and sometimes Result?

fn try_recv(&self) -> Result<T, TryRecvError>;
fn recv(&self) -> Option<T>;
fn next(&self) -> Option<T>;

Or, to put it differently, should recv return a Result too?

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Nov 27, 2019

@stjepang Interesting! None implies "no more items", where Result implies "something went wrong here". Arguably the try_ variants could return Option too, but then it wouldn't be possible to distinguish between error kinds.

On the other hand mpsc::Receiver::recv does return a Result. The way items are taken out without being fallible is by using iter, which returns None when done.

This is interesting; hadn't thought of this. I was going to say that the current state is good, but now looking at what std provides I'd almost say that following their lead might make more sense? crossbeam_channel::Receiver::recv works the same way too!

@ghost
Copy link

ghost commented Nov 27, 2019

Yeah... in that case, are you okay with recv() and next() (coming from Stream) differing in Result vs Option? They do the same thing, except recv is slightly more efficient that next.

fn recv(&self) -> Result<T, RecvError>;
fn next(&self) -> Option<T>;

@yoshuawuyts
Copy link
Contributor Author

@stjepang yeah, that sounds good. Seems to be the same in crossbeam and std as well (:

@yoshuawuyts
Copy link
Contributor Author

Note from triage: we should remove the Disconnected case from the TrySendError enum.

@leo60228
Copy link

Is this a good alternative?

use futures::prelude::FutureExt;

fn try_send<T>(chan: Sender<T>, x: T) -> bool {
    chan.send(x).now_or_never().is_some()
}

@yoshuawuyts
Copy link
Contributor Author

yoshuawuyts commented Dec 12, 2019

@stjepang actually I'm unsure how to remove that case. Maybe let's leave it in for now and consider changing it as part of stabilization?

edit: defined a new type RecvError that makes recv return a Result rather than an Option.

@yoshuawuyts
Copy link
Contributor Author

ping @stjepang

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

yes please, I was recently searching for eactly this

Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
Signed-off-by: Yoshua Wuyts <yoshuawuyts@gmail.com>
@yoshuawuyts
Copy link
Contributor Author

@dignifiedquire fixed the doctests, fingers crossed it passes now.

@yoshuawuyts yoshuawuyts merged commit 3ff9e98 into master Mar 16, 2020
@yoshuawuyts yoshuawuyts deleted the try_channels branch March 16, 2020 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose Receiver::try_recv API
4 participants