Skip to content

Commit

Permalink
std: Make std::comm return types consistent
Browse files Browse the repository at this point in the history
There are currently a number of return values from the std::comm methods, not
all of which are necessarily completely expressive:

  Sender::try_send(t: T) -> bool
    This method currently doesn't transmit back the data `t` if the send fails
    due to the other end having disconnected. Additionally, this shares the name
    of the synchronous try_send method, but it differs in semantics in that it
    only has one failure case, not two (the buffer can never be full).

  SyncSender::try_send(t: T) -> TrySendResult<T>
    This method accurately conveys all possible information, but it uses a
    custom type to the std::comm module with no convenience methods on it.
    Additionally, if you want to inspect the result you're forced to import
    something from `std::comm`.

  SyncSender::send_opt(t: T) -> Option<T>
    This method uses Some(T) as an "error value" and None as a "success value",
    but almost all other uses of Option<T> have Some/None the other way

  Receiver::try_recv(t: T) -> TryRecvResult<T>
    Similarly to the synchronous try_send, this custom return type is lacking in
    terms of usability (no convenience methods).

With this number of drawbacks in mind, I believed it was time to re-work the
return types of these methods. The new API for the comm module is:

  Sender::send(t: T) -> ()
  Sender::send_opt(t: T) -> Result<(), T>
  SyncSender::send(t: T) -> ()
  SyncSender::send_opt(t: T) -> Result<(), T>
  SyncSender::try_send(t: T) -> Result<(), TrySendError<T>>
  Receiver::recv() -> T
  Receiver::recv_opt() -> Result<T, ()>
  Receiver::try_recv() -> Result<T, TryRecvError>

The notable changes made are:

* Sender::try_send => Sender::send_opt. This renaming brings the semantics in
  line with the SyncSender::send_opt method. An asychronous send only has one
  failure case, unlike the synchronous try_send method which has two failure
  cases (full/disconnected).

* Sender::send_opt returns the data back to the caller if the send is guaranteed
  to fail. This method previously returned `bool`, but then it was unable to
  retrieve the data if the data was guaranteed to fail to send. There is still a
  race such that when `Ok(())` is returned the data could still fail to be
  received, but that's inherent to an asynchronous channel.

* Result is now the basis of all return values. This not only adds lots of
  convenience methods to all return values for free, but it also means that you
  can inspect the return values with no extra imports (Ok/Err are in the
  prelude). Additionally, it's now self documenting when something failed or not
  because the return value has "Err" in the name.

Things I'm a little uneasy about:

* The methods send_opt and recv_opt are not returning options, but rather
  results. I felt more strongly that Option was the wrong return type than the
  _opt prefix was wrong, and I coudn't think of a much better name for these
  methods. One possible way to think about them is to read the _opt suffix as
  "optionally".

* Result<T, ()> is often better expressed as Option<T>. This is only applicable
  to the recv_opt() method, but I thought it would be more consistent for
  everything to return Result rather than one method returning an Option.

Despite my two reasons to feel uneasy, I feel much better about the consistency
in return values at this point, and I think the only real open question is if
there's a better suffix for {send,recv}_opt.

Closes #11527
  • Loading branch information
alexcrichton committed Apr 11, 2014
1 parent 0156af1 commit 545d471
Show file tree
Hide file tree
Showing 27 changed files with 232 additions and 227 deletions.
7 changes: 3 additions & 4 deletions src/libgreen/sched.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1011,7 +1011,6 @@ fn new_sched_rng() -> XorShiftRng {
mod test {
use rustuv;

use std::comm;
use std::task::TaskOpts;
use std::rt::task::Task;
use std::rt::local::Local;
Expand Down Expand Up @@ -1428,7 +1427,7 @@ mod test {
// This task should not be able to starve the sender;
// The sender should get stolen to another thread.
spawn(proc() {
while rx.try_recv() != comm::Data(()) { }
while rx.try_recv().is_err() { }
});

tx.send(());
Expand All @@ -1445,7 +1444,7 @@ mod test {
// This task should not be able to starve the other task.
// The sends should eventually yield.
spawn(proc() {
while rx1.try_recv() != comm::Data(()) {
while rx1.try_recv().is_err() {
tx2.send(());
}
});
Expand Down Expand Up @@ -1499,7 +1498,7 @@ mod test {
let mut val = 20;
while val > 0 {
val = po.recv();
ch.try_send(val - 1);
let _ = ch.send_opt(val - 1);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/libgreen/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ mod tests {
let _tx = tx;
fail!()
});
assert_eq!(rx.recv_opt(), None);
assert_eq!(rx.recv_opt(), Err(()));
}

#[test]
Expand Down
9 changes: 4 additions & 5 deletions src/libnative/io/timer_other.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
//!
//! Note that all time units in this file are in *milliseconds*.

use std::comm::Data;
use libc;
use std::mem;
use std::os;
Expand Down Expand Up @@ -119,7 +118,7 @@ fn helper(input: libc::c_int, messages: Receiver<Req>) {
Some(timer) => timer, None => return
};
let tx = timer.tx.take_unwrap();
if tx.try_send(()) && timer.repeat {
if tx.send_opt(()).is_ok() && timer.repeat {
timer.tx = Some(tx);
timer.target += timer.interval;
insert(timer, active);
Expand Down Expand Up @@ -162,14 +161,14 @@ fn helper(input: libc::c_int, messages: Receiver<Req>) {
1 => {
loop {
match messages.try_recv() {
Data(Shutdown) => {
Ok(Shutdown) => {
assert!(active.len() == 0);
break 'outer;
}

Data(NewTimer(timer)) => insert(timer, &mut active),
Ok(NewTimer(timer)) => insert(timer, &mut active),

Data(RemoveTimer(id, ack)) => {
Ok(RemoveTimer(id, ack)) => {
match dead.iter().position(|&(i, _)| id == i) {
Some(i) => {
let (_, i) = dead.remove(i).unwrap();
Expand Down
9 changes: 4 additions & 5 deletions src/libnative/io/timer_timerfd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
//!
//! As with timer_other, all units in this file are in units of millseconds.

use std::comm::Data;
use libc;
use std::ptr;
use std::os;
Expand Down Expand Up @@ -107,7 +106,7 @@ fn helper(input: libc::c_int, messages: Receiver<Req>) {
match list.as_slice().bsearch(|&(f, _, _)| f.cmp(&fd)) {
Some(i) => {
let (_, ref c, oneshot) = *list.get(i);
(!c.try_send(()) || oneshot, i)
(c.send_opt(()).is_err() || oneshot, i)
}
None => fail!("fd not active: {}", fd),
}
Expand All @@ -121,7 +120,7 @@ fn helper(input: libc::c_int, messages: Receiver<Req>) {

while incoming {
match messages.try_recv() {
Data(NewTimer(fd, chan, one, timeval)) => {
Ok(NewTimer(fd, chan, one, timeval)) => {
// acknowledge we have the new channel, we will never send
// another message to the old channel
chan.send(());
Expand Down Expand Up @@ -149,7 +148,7 @@ fn helper(input: libc::c_int, messages: Receiver<Req>) {
assert_eq!(ret, 0);
}

Data(RemoveTimer(fd, chan)) => {
Ok(RemoveTimer(fd, chan)) => {
match list.as_slice().bsearch(|&(f, _, _)| f.cmp(&fd)) {
Some(i) => {
drop(list.remove(i));
Expand All @@ -160,7 +159,7 @@ fn helper(input: libc::c_int, messages: Receiver<Req>) {
chan.send(());
}

Data(Shutdown) => {
Ok(Shutdown) => {
assert!(list.len() == 0);
break 'outer;
}
Expand Down
9 changes: 4 additions & 5 deletions src/libnative/io/timer_win32.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
//! Other than that, the implementation is pretty straightforward in terms of
//! the other two implementations of timers with nothing *that* new showing up.

use std::comm::Data;
use libc;
use std::ptr;
use std::rt::rtio;
Expand Down Expand Up @@ -54,11 +53,11 @@ fn helper(input: libc::HANDLE, messages: Receiver<Req>) {
if idx == 0 {
loop {
match messages.try_recv() {
Data(NewTimer(obj, c, one)) => {
Ok(NewTimer(obj, c, one)) => {
objs.push(obj);
chans.push((c, one));
}
Data(RemoveTimer(obj, c)) => {
Ok(RemoveTimer(obj, c)) => {
c.send(());
match objs.iter().position(|&o| o == obj) {
Some(i) => {
Expand All @@ -68,7 +67,7 @@ fn helper(input: libc::HANDLE, messages: Receiver<Req>) {
None => {}
}
}
Data(Shutdown) => {
Ok(Shutdown) => {
assert_eq!(objs.len(), 1);
assert_eq!(chans.len(), 0);
break 'outer;
Expand All @@ -79,7 +78,7 @@ fn helper(input: libc::HANDLE, messages: Receiver<Req>) {
} else {
let remove = {
match chans.get(idx as uint - 1) {
&(ref c, oneshot) => !c.try_send(()) || oneshot
&(ref c, oneshot) => c.send_opt(()).is_err() || oneshot
}
};
if remove {
Expand Down
2 changes: 1 addition & 1 deletion src/libnative/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ mod tests {
let _tx = tx;
fail!()
});
assert_eq!(rx.recv_opt(), None);
assert_eq!(rx.recv_opt(), Err(()));
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/librustuv/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1065,7 +1065,7 @@ mod test {
}
reads += 1;

tx2.try_send(());
let _ = tx2.send_opt(());
}

// Make sure we had multiple reads
Expand Down
2 changes: 1 addition & 1 deletion src/librustuv/signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl SignalWatcher {
extern fn signal_cb(handle: *uvll::uv_signal_t, signum: c_int) {
let s: &mut SignalWatcher = unsafe { UvHandle::from_uv_handle(&handle) };
assert_eq!(signum as int, s.signal as int);
s.channel.try_send(s.signal);
let _ = s.channel.send_opt(s.signal);
}

impl HomingIO for SignalWatcher {
Expand Down
12 changes: 6 additions & 6 deletions src/librustuv/timer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ extern fn timer_cb(handle: *uvll::uv_timer_t, status: c_int) {
let task = timer.blocker.take_unwrap();
let _ = task.wake().map(|t| t.reawaken());
}
SendOnce(chan) => { let _ = chan.try_send(()); }
SendOnce(chan) => { let _ = chan.send_opt(()); }
SendMany(chan, id) => {
let _ = chan.try_send(());
let _ = chan.send_opt(());

// Note that the above operation could have performed some form of
// scheduling. This means that the timer may have decided to insert
Expand Down Expand Up @@ -196,8 +196,8 @@ mod test {
let oport = timer.oneshot(1);
let pport = timer.period(1);
timer.sleep(1);
assert_eq!(oport.recv_opt(), None);
assert_eq!(pport.recv_opt(), None);
assert_eq!(oport.recv_opt(), Err(()));
assert_eq!(pport.recv_opt(), Err(()));
timer.oneshot(1).recv();
}

Expand Down Expand Up @@ -284,7 +284,7 @@ mod test {
let mut timer = TimerWatcher::new(local_loop());
timer.oneshot(1000)
};
assert_eq!(port.recv_opt(), None);
assert_eq!(port.recv_opt(), Err(()));
}

#[test]
Expand All @@ -293,7 +293,7 @@ mod test {
let mut timer = TimerWatcher::new(local_loop());
timer.period(1000)
};
assert_eq!(port.recv_opt(), None);
assert_eq!(port.recv_opt(), Err(()));
}

#[test]
Expand Down
Loading

5 comments on commit 545d471

@bors
Copy link
Contributor

@bors bors commented on 545d471 Apr 12, 2014

Choose a reason for hiding this comment

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

saw approval from brson
at alexcrichton@545d471

@bors
Copy link
Contributor

@bors bors commented on 545d471 Apr 12, 2014

Choose a reason for hiding this comment

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

merging alexcrichton/rust/rework-chan-return-values = 545d471 into auto

@bors
Copy link
Contributor

@bors bors commented on 545d471 Apr 12, 2014

Choose a reason for hiding this comment

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

alexcrichton/rust/rework-chan-return-values = 545d471 merged ok, testing candidate = ab0d847

@bors
Copy link
Contributor

@bors bors commented on 545d471 Apr 12, 2014

@bors
Copy link
Contributor

@bors bors commented on 545d471 Apr 12, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = ab0d847

Please sign in to comment.