Implement RFC 458: improve Send #22319

Merged
merged 6 commits into from Feb 18, 2015

Conversation

Projects
None yet
7 participants
@huonw
Member

huonw commented Feb 14, 2015

This removes the 'static bound from Send, and implements Send for &'a T ( T: Sync) and &'a mut T (T: Send), as per rust-lang/rfcs#458, and so closes #22251.

This is rebased on top of Niko's default object lifetime work.


This makes a parallel for function like

fn par_for<I, F>(iter: I, f: F)
    where I: Iterator,
          <I as Iterator>::Item: Send,
          F: Fn(<I as Iterator>::Item) + Sync
{
    let _guards: Vec<_> = iter.map(|elem| {
        Thread::scoped(|| {
            f(elem)
        })
    }).collect();

}

compile, run in parallel, and work with essentially arbitrary iterators, including over references of arrays stored on the stack etc.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Feb 14, 2015

Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Feb 14, 2015

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Feb 14, 2015

Member

r? @nikomatsakis


This isn't quite ready for merging, since this compiles, but it seems like it shouldn't, since y is a pointer into the stack frame of the bad closure, but the join guard is being used outside bad, it seems the lifetime restriction in Thread::scoped should stop the join guard lasting longer than the closure itself.

#![feature(core, std_misc)]
use std::thread::Thread;


fn main() {
    let bad = || {
        let x = 1;
        let y = &x;

        Thread::scoped(|| {
            let z = y;
            1 + *z
        })
    };

    println!("{}", bad().join().ok().unwrap());
}
Member

huonw commented Feb 14, 2015

r? @nikomatsakis


This isn't quite ready for merging, since this compiles, but it seems like it shouldn't, since y is a pointer into the stack frame of the bad closure, but the join guard is being used outside bad, it seems the lifetime restriction in Thread::scoped should stop the join guard lasting longer than the closure itself.

#![feature(core, std_misc)]
use std::thread::Thread;


fn main() {
    let bad = || {
        let x = 1;
        let y = &x;

        Thread::scoped(|| {
            let z = y;
            1 + *z
        })
    };

    println!("{}", bad().join().ok().unwrap());
}

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Feb 14, 2015

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Feb 14, 2015

Member

This also does essentially no auditing, just mindlessly adding an additional 'static bound to most instances of Send in std, except in std::thread and std::thunk.

cc @pythonesque, @aturon.

Member

huonw commented Feb 14, 2015

This also does essentially no auditing, just mindlessly adding an additional 'static bound to most instances of Send in std, except in std::thread and std::thunk.

cc @pythonesque, @aturon.

@pythonesque

This comment has been minimized.

Show comment
Hide comment
@pythonesque

pythonesque Feb 14, 2015

Contributor

I take it that after auditing, the expectation is that we'll be able to remove 'static from a lot of the other types as well?

Contributor

pythonesque commented Feb 14, 2015

I take it that after auditing, the expectation is that we'll be able to remove 'static from a lot of the other types as well?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 16, 2015

Member

Could this actually continue to have 'static on std::thread in light of #20807?

Member

alexcrichton commented Feb 16, 2015

Could this actually continue to have 'static on std::thread in light of #20807?

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

Exciting! I'll take a look, and in particular take a look at the unsafety errors.

Contributor

nikomatsakis commented Feb 16, 2015

Exciting! I'll take a look, and in particular take a look at the unsafety errors.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

@alexcrichton

Could this actually continue to have 'static on std::thread in light of #20807?

I personally think that'd be rather disappointing. We'd be giving up quite a lot of expressiveness (and gaining that expressiveness was sort of the point of the changes to Send/Sync).

Contributor

nikomatsakis commented Feb 16, 2015

@alexcrichton

Could this actually continue to have 'static on std::thread in light of #20807?

I personally think that'd be rather disappointing. We'd be giving up quite a lot of expressiveness (and gaining that expressiveness was sort of the point of the changes to Send/Sync).

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Feb 16, 2015

Member

Re-adding 'static until #20807 is resolved doesn't lose anything compared to our current libs...

Then again, I do want my safe parallel_map right now! :P

Member

huonw commented Feb 16, 2015

Re-adding 'static until #20807 is resolved doesn't lose anything compared to our current libs...

Then again, I do want my safe parallel_map right now! :P

@pythonesque

This comment has been minimized.

Show comment
Hide comment
@pythonesque

pythonesque Feb 16, 2015

Contributor

@huonw Looking at your example, I'm not fully convinced it's unsafe. Doesn't the closure last until bad itself goes out of scope? I don't think it has at the point where you use the join guard. I could easily be confused, though (_edit:_ yep, pretty sure I was just confused :P)

Contributor

pythonesque commented Feb 16, 2015

@huonw Looking at your example, I'm not fully convinced it's unsafe. Doesn't the closure last until bad itself goes out of scope? I don't think it has at the point where you use the join guard. I could easily be confused, though (_edit:_ yep, pretty sure I was just confused :P)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

@huonw that's true, it doesn't lose anything relative to now. I'm just not happy with it as a permanent solution (though I am fine if we wind up with a different API).

Anyway, the problem you encountered is, I think, is the fact that 'a in JoinGuard is not used anywhere. This was fixed in the variance PR (since an unconstrained lifetime is a hard error there). But in the meantime adding this patch makes your example work fine:

lunch-box. git diff libstd/
diff --git a/src/libstd/thread.rs b/src/libstd/thread.rs
index 3b758c8..f291e83 100644
--- a/src/libstd/thread.rs
+++ b/src/libstd/thread.rs
@@ -255,6 +255,7 @@ impl Builder {
             joined: false,
             packet: my_packet,
             thread: thread,
+            _marker: [],
         }
     }

@@ -487,6 +488,7 @@ pub struct JoinGuard<'a, T: 'a> {
     thread: Thread,
     joined: bool,
     packet: Packet<T>,
+    _marker: [&'a T; 0], // bind the lifetime `'a`
 }

 #[stable(feature = "rust1", since = "1.0.0")]

(As an aside I recently realized that [T;0] is a "poor man's" phantom data, in that it has precisely the same effect.)

Contributor

nikomatsakis commented Feb 16, 2015

@huonw that's true, it doesn't lose anything relative to now. I'm just not happy with it as a permanent solution (though I am fine if we wind up with a different API).

Anyway, the problem you encountered is, I think, is the fact that 'a in JoinGuard is not used anywhere. This was fixed in the variance PR (since an unconstrained lifetime is a hard error there). But in the meantime adding this patch makes your example work fine:

lunch-box. git diff libstd/
diff --git a/src/libstd/thread.rs b/src/libstd/thread.rs
index 3b758c8..f291e83 100644
--- a/src/libstd/thread.rs
+++ b/src/libstd/thread.rs
@@ -255,6 +255,7 @@ impl Builder {
             joined: false,
             packet: my_packet,
             thread: thread,
+            _marker: [],
         }
     }

@@ -487,6 +488,7 @@ pub struct JoinGuard<'a, T: 'a> {
     thread: Thread,
     joined: bool,
     packet: Packet<T>,
+    _marker: [&'a T; 0], // bind the lifetime `'a`
 }

 #[stable(feature = "rust1", since = "1.0.0")]

(As an aside I recently realized that [T;0] is a "poor man's" phantom data, in that it has precisely the same effect.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

Also this is a simpler example that exhibited the same problem (no need for closures):

#![feature(core, std_misc)]
use std::thread::Thread;

fn main() {
    let bad = {
        let x = 1;
        let y = &x;

        Thread::scoped(|| {
            let z = y;
            1 + *z
        })
    };

    println!("{}", bad.join().ok().unwrap());
}
Contributor

nikomatsakis commented Feb 16, 2015

Also this is a simpler example that exhibited the same problem (no need for closures):

#![feature(core, std_misc)]
use std::thread::Thread;

fn main() {
    let bad = {
        let x = 1;
        let y = &x;

        Thread::scoped(|| {
            let z = y;
            1 + *z
        })
    };

    println!("{}", bad.join().ok().unwrap());
}
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

I added a few comments of places I do not think the 'static bound is needed -- basically wrappers around types, like Mutex, Future, and channels should be able to do without. But it may be worth loosening those in a separate PR, as I imagine we'd want to write some run-pass and compile-fail tests checking that the type system is enforcing that you don't leak references.

Contributor

nikomatsakis commented Feb 16, 2015

I added a few comments of places I do not think the 'static bound is needed -- basically wrappers around types, like Mutex, Future, and channels should be able to do without. But it may be worth loosening those in a separate PR, as I imagine we'd want to write some run-pass and compile-fail tests checking that the type system is enforcing that you don't leak references.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 16, 2015

Member

@alexcrichton

Could this actually continue to have 'static on std::thread in light of #20807?

I personally think that'd be rather disappointing. We'd be giving up quite a lot of expressiveness (and gaining that expressiveness was sort of the point of the changes to Send/Sync).

Oh sorry I should have mentioned that I, too, would love to remove the 'static bound. I was just thinking in terms of "we may tweak the API slightly" but it's #[unstable] anyway so it's fine either way.

Member

alexcrichton commented Feb 16, 2015

@alexcrichton

Could this actually continue to have 'static on std::thread in light of #20807?

I personally think that'd be rather disappointing. We'd be giving up quite a lot of expressiveness (and gaining that expressiveness was sort of the point of the changes to Send/Sync).

Oh sorry I should have mentioned that I, too, would love to remove the 'static bound. I was just thinking in terms of "we may tweak the API slightly" but it's #[unstable] anyway so it's fine either way.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

well, modulo the need for a marker on JoinGuard, the PR looks good to me. I think it makes sense to leave FIXMEs for the 'static bounds I identified as being ones we could loosen, and then come back and make sure we have pos/neg tests for them.

Contributor

nikomatsakis commented Feb 16, 2015

well, modulo the need for a marker on JoinGuard, the PR looks good to me. I think it makes sense to leave FIXMEs for the 'static bounds I identified as being ones we could loosen, and then come back and make sure we have pos/neg tests for them.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Feb 16, 2015

Contributor

(I'm somewhat indifferent on whether we take that same approach for JoinGuard or not.)

Contributor

nikomatsakis commented Feb 16, 2015

(I'm somewhat indifferent on whether we take that same approach for JoinGuard or not.)

@japaric

This comment has been minimized.

Show comment
Hide comment
@japaric

japaric Feb 16, 2015

Member

Shouldn't _marker prefer PhantomData<&'a T> over [&'a T; 0]?

Anyway, I'm excited to get sendable references/slices 😍.

Member

japaric commented Feb 16, 2015

Shouldn't _marker prefer PhantomData<&'a T> over [&'a T; 0]?

Anyway, I'm excited to get sendable references/slices 😍.

huonw added some commits Feb 13, 2015

Remove the implicit `'static` bound on `Send`.
Previously Send was defined as `trait Send: 'static {}`. As detailed in
rust-lang/rfcs#458, the `'static` bound is not
actually necessary for safety, we can use lifetimes to enforce that more
flexibly.

`unsafe` code that was previously relying on `Send` to insert a
`'static` bound now may allow incorrect patterns, and so should be
audited (a quick way to ensure safety immediately and postpone the audit
is to add an explicit `'static` bound to any uses of the `Send` type).

cc #22251.
Update the libraries to reflect Send loosing the 'static bound.
In most places this preserves the current API by adding an explicit
`'static` bound.

Notably absent are some impls like `unsafe impl<T: Send> Send for
Foo<T>` and the `std::thread` module. It is likely that it will be
possible to remove these after auditing the code to ensure restricted
lifetimes are safe.

More progress on #22251.
Add missing marker to std::thread::JoinGuard.
The lifetime was previously, incorrectly unconstrained.

@huonw huonw referenced this pull request Feb 17, 2015

Merged

Revise std::thread #22435

@huonw

This comment has been minimized.

Show comment
Hide comment
@huonw

huonw Feb 18, 2015

Member

Updated, tests pass.

Member

huonw commented Feb 18, 2015

Updated, tests pass.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
Member

alexcrichton commented Feb 18, 2015

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 18, 2015

rollup merge of #22319: huonw/send-is-not-static
Conflicts:
	src/libstd/sync/task_pool.rs
	src/libstd/thread.rs
	src/libtest/lib.rs
	src/test/bench/shootout-reverse-complement.rs
	src/test/bench/shootout-spectralnorm.rs

@huonw huonw merged commit 7a14f49 into rust-lang:master Feb 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@teotwaki teotwaki referenced this pull request in reem/rust-event Feb 20, 2015

Merged

Fixes for nightly #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment