dpc / mioco.pre-0.9

Scalable, coroutine-based, asynchronous IO handling library for Rust programming language. (aka MIO COroutines).
Mozilla Public License 2.0
457 stars 30 forks source link

`Receiver` should not be always `Send` #132

Closed SAPikachu closed 8 years ago

SAPikachu commented 8 years ago

At https://github.com/dpc/mioco/blob/master/src/sync/mpsc.rs#L245 , Receiver is always marked as Send regardless of what T is. This may allow non-thread-safe objects leaking to different threads, which may break thread-safety. I think we should mark it as Send only when T is also Send?

dpc commented 8 years ago

I need to look at it, but you're probably right.

SAPikachu commented 8 years ago

~~Just for your reference, here is one case that is problematic I think: https://github.com/SAPikachu/euphonium/blob/8ff0429c1f3946e611af487be400e32a4d332300/src/resolver.rs#L354 In the method above, I moved Receiver<Query> into a new coroutine, but Query contains Rc, that implies Query is not Send, so it shouldn't be used in multiple threads.~~

After checking the code I found that Rc in Query is actually renamed from Arc, so my comment above was not correct, oops. Sorry for the confusion.

SAPikachu commented 8 years ago

Just wrote a minimal test case to demonstrate the issue:

extern crate mioco;

use std::rc::Rc;

use mioco::sync::mpsc::{channel};

fn main() {
    mioco::start(move || {
        let x = Rc::<i32>::new(42);
        let (send, recv) = channel::<Rc<i32>>();
        let handle = mioco::spawn(move || {
            println!("Received: {}", recv.recv().unwrap());
        });
        send.send(x.clone());
        println!("Sent: {}", x);
        handle.join();
    });
}

This snippet compiles without error, but it shouldn't as this (potentially) leaks Rc to a new thread.

dpc commented 8 years ago

Should be fixed now.

SAPikachu commented 8 years ago

Thanks!