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

mpsc::send returns error once, then success #32114

Closed
jsgf opened this issue Mar 8, 2016 · 4 comments
Closed

mpsc::send returns error once, then success #32114

jsgf opened this issue Mar 8, 2016 · 4 comments

Comments

@jsgf
Copy link
Contributor

jsgf commented Mar 8, 2016

If you close the rx side of a channel pair before sending to it, then the first send will fail with a SendError as expected, but a second send will return Ok(()).

use std::sync::mpsc::{channel, SendError};

#[test]
fn doublesend() {
    let (tx, _) = channel();

    assert_eq!(tx.send(123), Ok(()));
    assert_eq!(tx.send(123), Err(SendError(123)));
    assert_eq!(tx.send(123), Err(SendError(123)));
}

Fails with:

thread 'doublesend' panicked at 'assertion failed: (left == right) (left: Err("SendError(..)"), right: Ok(()))', :7

However, if something is successfully sent before the receiver is dropped (even if it never receives the value), then it works as expected:

use std::sync::mpsc::{channel, SendError};
use std::mem;

#[test]
fn doublesend() {
    let (tx, rx) = channel();

    assert_eq!(tx.send(123), Ok(()));
    mem::drop(rx);
    assert_eq!(tx.send(123), Err(SendError(123)));
    assert_eq!(tx.send(123), Err(SendError(123)));
}
@alexcrichton
Copy link
Member

I may be a bit confused, but it looks like everything is working as intended here? In the first example the receiver is immediately dropped (binding via _), and the first send fails (line 7) with an error appropriately because there is no receiver to receive the message.

The second example also looks correct as when the first message is sent the receiver is still active (could possibly acquire the message), but afterwards it's guaranteed to never be received so the next two sends are errors.

@bluss
Copy link
Member

bluss commented Mar 8, 2016

@alexcrichton
Copy link
Member

Aha! So to inline the problem:

use std::sync::mpsc::{channel, SendError};

fn main() {
    let (tx, _) = channel();

    let _ = tx.send(123);
    assert_eq!(tx.send(123), Err(SendError(123)));
}

Yes, this is indeed unexpected behavior! It's probably something to do with the upgrade logic in channels.

@apasel422
Copy link
Contributor

I think the issue here is that oneshot::Packet::send unconditionally sets the state field to DATA, even when the subsequent match indicates that the state was DISCONNECTED. This causes the first send to error correctly, but causes the second to succeed.

bors added a commit that referenced this issue Oct 6, 2016
Restore `DISCONNECTED` state in `oneshot::Packet::send`

Closes #32114

I'm not sure if this is the best approach, but the current action of swapping `DISCONNECTED` with `DATA` seems wrong. Additionally, it is strange that the `send` method (and others in the `oneshot` module) takes `&mut self` despite performing atomic operations, as this requires extra discipline to avoid data races and lets us use methods like `AtomicUsize::get_mut` instead of methods that require a memory ordering.
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

No branches or pull requests

4 participants