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

Should Sender::try_send return the message on failure? #11527

Closed
brson opened this issue Jan 14, 2014 · 4 comments · Fixed by #13448
Closed

Should Sender::try_send return the message on failure? #11527

brson opened this issue Jan 14, 2014 · 4 comments · Fixed by #13448
Labels
A-concurrency Area: Concurrency
Milestone

Comments

@brson
Copy link
Contributor

brson commented Jan 14, 2014

If we add a synchronous channel it will need to return the message if the queue is full, and it might as well return the message if the channel is closed. Seems useful to be able to retrieve the message in all cases where a send doesn't succeed.

Nominating.

@liigo
Copy link
Contributor

liigo commented Jan 14, 2014

+1

@thestinger
Copy link
Contributor

Here's the API used by rust-core for multiple-producer/multiple-consumer bounded queues:

/// Return a new `BoundedPriorityQueue` instance, holding at most `maximum` elements.
fn new(maximum: uint) -> BoundedPriorityQueue<A>;

/// Pop the largest value from the queue, blocking until the queue is not empty
fn pop(&self) -> A;

/// Pop the largest value from the queue, or return `None` if the queue is empty.
fn try_pop(&self) -> Option<A>;

/// Pop the largest value from the queue, blocking until the queue is not empty or the timeout
/// expires.
fn pop_timeout(&self, reltime: Time) -> Option<A>;

/// Push a value into the queue, blocking until the queue is not full.
fn push(&self, item: A);

/// Push a value into the queue, or return `Some(item)` if the queue is full.
fn try_push(&self, item: A) -> Option<A>;

/// Push a value into the queue, blocking until the queue is not full or the timeout expires. If
/// the timeout expires, return `Some(item)`.
fn push_timeout(&self, item: A, reltime: Time) -> Option<A>;

There's no split in the types or a closed error case though. That's required if there's going to be a limitation of one consumer, but I don't think a micro-optimization belongs in the default channel you reach for.

In my opinion, a multiple-producer/multiple-consumer queue with support for blocking is the most important type as it supports the most functionality. You can swap it out for a less flexible queue with a single consumer and/or single producer limitation if possible but you're not forced to change your design to accommodate a micro-optimization that's not going to be a measurable difference in most cases. You can also switch to a purely non-blocking queue like the disruptor.

The more restricted forms would be included only after they were shown to offer performance improvements. At the moment, the blocking queue in Thread Building Blocks is significantly faster than Rust's channels despite support multiple consumers with fair scheduling and an optional size bound.

@pnkfelix
Copy link
Member

Accepted for P-backcompat-libs (though we may or may not decide to actually do it).

@nikomatsakis
Copy link
Contributor

My feeling is that full vs closed are distinct; it makes sense to get back the message if full, but not necessarily closed, since that may not be detectable and even if the message is enqeued, you don't know that the receiver will read it from the port.

@alexcrichton alexcrichton changed the title Should Chan::try_send return the message on failure? Should Sender::try_send return the message on failure? Apr 10, 2014
alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 11, 2014
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 rust-lang#11527
bors added a commit that referenced this issue Apr 12, 2014
…=brson

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
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 6, 2023
new lint: `iter_without_into_iter`

Closes rust-lang#9736

A new lint that looks for `iter` (and `iter_mut`) method implementations without the type implementing `IntoIterator` for `&Type`.
Imo this seems rather pedantic, so I went with that, but I can be convinced to change it to `style` like the linked issue asked for.
Writing a machine applicable suggestion seems a bit tricky and tedious, so for now this relies on the user adding remaining lifetimes.

changelog: new lint: `iter_without_into_iter`
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 6, 2023
new lint: `into_iter_without_iter`

Closes rust-lang#9736 (part 2)

This implements the other lint that my earlier PR missed: given an `IntoIterator for &Type` impl, check that there exists an inherent `fn iter(&self)` method.

changelog: new lint: `into_iter_without_iter`

r? `@Jarcho` since you reviewed rust-lang#11527 I figured it makes sense for you to review this as well?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-concurrency Area: Concurrency
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants