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

LocalSend instead of Send #66

Merged
merged 1 commit into from Jul 5, 2016
Merged

Conversation

therustmonk
Copy link
Contributor

Proposal to fix #31
With this PR I can compile simple example, but can't compile tls example:

extern crate coroutine;

use coroutine::asymmetric::Coroutine;
use std::cell::RefCell;

thread_local!(static LOCAL: RefCell<u32> = RefCell::new(7));

fn main() {
    let mut handle = Coroutine::spawn(move|cor| {
        LOCAL.with(|x| {
            *x.borrow_mut() += 1;
            let ptr: *const RefCell<u32> = x;
            println!("before: {:?} {:?} thread: {:?}", ptr, x, std::thread::current());
            cor.yield_with(0);
            let ptr: *const RefCell<u32> = x;
            println!("after: {:?} {:?} thread: {:?}", ptr, x, std::thread::current());
        });
    });

    (*handle).yield_with(0);

    std::thread::spawn(move || {
        (*handle).yield_with(0);
    }).join().unwrap();
}

Fails with:

   Compiling coroutine v0.5.0 (file:///home/denis/workspace/github.com/coroutine-rs)
examples/tls.rs:22:5: 22:23 error: the trait bound `*mut coroutine::asymmetric::Coroutine: std::marker::Send` is not satisfied [E0277]
examples/tls.rs:22     std::thread::spawn(move || {
                       ^~~~~~~~~~~~~~~~~~
examples/tls.rs:22:5: 22:23 help: run `rustc --explain E0277` to see a detailed explanation
examples/tls.rs:22:5: 22:23 note: `*mut coroutine::asymmetric::Coroutine` cannot be sent between threads safely
examples/tls.rs:22:5: 22:23 note: required because it appears within the type `coroutine::asymmetric::Handle`
examples/tls.rs:22:5: 22:23 note: required because it appears within the type `[closure@examples/tls.rs:22:24: 24:6 handle:coroutine::asymmetric::Handle]`
examples/tls.rs:22:5: 22:23 note: required by `std::thread::spawn`
error: aborting due to previous error
error: Could not compile `coroutine`.

@therustmonk
Copy link
Contributor Author

Second commit makes possible the following:

extern crate coroutine;

use std::rc::Rc;
use std::cell::RefCell;
use coroutine::asymmetric::Coroutine;

fn main() {

    let rc = Rc::new(RefCell::new(0));

    let rc1 = rc.clone();
    let mut coro1 = Coroutine::spawn(move |me| {
        *rc1.borrow_mut() = 1;
        let val = *rc1.borrow();
        me.yield_with(val); // (*rc1.borrow()) - fails with already borrowed
    });

    let rc2 = rc.clone();
    let mut coro2 = Coroutine::spawn(move |me| {
        *rc2.borrow_mut() = 2;
        let val = *rc2.borrow();
        me.yield_with(val);
    });

    println!("First: {}", (*coro1).yield_with(0));
    println!("Second: {}", (*coro2).yield_with(0));
}

@therustmonk
Copy link
Contributor Author

@zonyitoo Is there any reason we have to leave Send impls?

@zonyitoo
Copy link
Contributor

zonyitoo commented Jul 5, 2016

Well, it is still unsafe because you just tried to bypass the check. I cannot remember why I added Send bound for the closure originally, maybe it is not required for the current rustc version.

@zonyitoo
Copy link
Contributor

zonyitoo commented Jul 5, 2016

If you are interested in building a library that would migrate coroutines between threads (which is the only reason why you care about TLS safety), why not join us to improve coio-rs.

@therustmonk
Copy link
Contributor Author

@zonyitoo No, I won't use library with threads. I'm moving simulation Lua's library to Rust and want to use coroutines inside one thread only. But to use reference counters without overhead (Rc, no Arc) I need reject restriction to implement Send.

I've add TLS example to show: Send removing repairs checks and adds two profits:

  • Impossible to break TLS
  • Rc, RefCell can be used

I'll squash the commits, remove tls example, and Let's check compatibility with other libs using this. If nothing breaks why we have to still using Send restriction?!

What libraries I need to backward compatibility check with this PR?

@therustmonk
Copy link
Contributor Author

Build successful! I've also checked it with my simulator. It works well without Send. 👍

@zonyitoo
Copy link
Contributor

zonyitoo commented Jul 5, 2016

None, I really don't know who is using this library. This library was built as a base library for coio-rs and mioco, and all of us use our own encapsulation upon context-rs now.

I will merge this PR, and lets see who will complain about it.

@zonyitoo zonyitoo merged commit a8f93a0 into rustcc:master Jul 5, 2016
@therustmonk
Copy link
Contributor Author

@zonyitoo Thank you!

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.

Using Thread local storage is unsafe
2 participants