Implements a more idiomatic thread result without using mem::uninitialized() and the associated undefined behaviour #35

oliver-giersch · 2018-07-26T21:25:51Z

Implements a ScopedThreadResult struct that stores a raw pointer that can be shared with a scoped thread, while avoiding mem::uninitialized() and mem::ManuallyForget<T>.

…inter between the scoped thread and the join handles, that avoids use of mem::ManuallyForget and mem::uninitialized().

oliver-giersch · 2018-07-26T21:27:23Z

This should solve issue #34 (further discussion in #27).

I am not 100% sure if the Drop implementation for ScopedThreadResult is strictly necessary, as the deferred join() call should deallocate the Box even when a thread panicks.

ghost

Thanks for the PR! Yay, this will finally resolve the remaining safety issues! 👍

I've written some comments inline. There are a few safety issues still left.

But here's a suggestion for an alternative solution: we can eliminate a lot of tricky unsafe code by using Arc<Mutex<Option<T>>> instead of raw pointers.

First, we change the definition of JoinState so that result becomes an Arc<Mutex<Option<T>>>:

struct JoinState<T> {
    join_handle: thread::JoinHandle<()>,
    result: Arc<Mutex<Option<T>>>,
}

Next, we change join:

impl<T: Send> JoinState<T> {
    fn join(self) -> thread::Result<T> {
        self.join_handle
            .join()
            .map(|_| self.result().lock().unwrap().take().unwrap())
    }
}

And finally, the relevant part in spawn:

let result = Arc::new(Mutex::new(None));

let join_handle = unsafe {
    let result = result.clone();
    builder_spawn_unchecked(self.builder, move || {
        let r = f();
        *result.lock().unwrap() = Some(r);
    })?
};

That's all. Simple and almost no unsafe code. :)

Now there's some cost due to reference counting and locking, but there's no contention on the mutex and the overall cost is totally negligible.

ghost · 2018-07-26T21:45:03Z

src/thread.rs

+    fn drop(&mut self) {
+        let _ = Box::from(self.pointer.as_ptr());
+    }
+}


Nit: missing newline.

ghost · 2018-07-26T21:45:34Z

src/thread.rs

+impl<T> Drop for ScopedThreadResult<T> {
+    #[inline]
+    fn drop(&mut self) {
+        let _ = Box::from(self.pointer.as_ptr());


This line allocates the pointer on the heap. :)
We should use Box::from_raw instead.

Yeah that's a stupid mistake on my part, but as you also correctly deduced, Drop should probably either be scratched or the into_inner() method should not deallocate the Box but just deference the pointer and then let the Drop impl handle the deallocation.

ghost · 2018-07-26T21:56:44Z

src/thread.rs

-                let mut result = Box::from_raw(result as *mut ManuallyDrop<T>);
-                *result = ManuallyDrop::new(f());
-                mem::forget(result);
+                thread_result.set(f());


If f() panics, we'll drop the ScopedThreadResult twice. Once here in this closure and the other time inside .join(). This is a double-free. Hmmm, we had the same problem in the previous implementation, didn't we? :/

The easiest way of fixing this would probably be to remove the Drop implementation for ScopedThreadResult and manually deallocate inside join().

I don't think the current implementation does a double free, due to the use of mem::ManuallyDrop (I think at least).

ghost · 2018-07-26T22:01:17Z

src/thread.rs

+/// implementing Send & Sync. This is only safe to use with scoped threads.
+#[derive(Debug)]
+struct ScopedThreadResult<T> {
+    pointer: NonNull<Option<T>>


I'm pretty sure this needs to be NonNull<UnsafeCell<Option<T>>> (for the same reason why e.g. Mutex<T> and RefCell<T> need UnsafeCell).

No, NonNull is just a wrapper around a pointer, so it can be copied and cloned and doesn't need interior mutability. This is all unsafe though, ofc.

In fact, you were right. By copying the ScopedThreadResult into the thread it also got dropped at the end of the closure, rendering the entire thing unsound. I have implement the safe variant wit Arc and Mutex instead for now and maybe try around a little more with an unsafe variant later.

oliver-giersch · 2018-07-27T10:57:10Z

Will get to the necessary fixes later. There is a lot to be said in favor of safe code over unsafe code, but using an Arc<Mutex<_>> for this feels a bit like overkill if you ask me.

…t Drop) in favor of a safe solution using std::sync::Arc and std::sync::Mutex

ghost

Two unsafe blocks fewer. I like this. 👍

@jeehoonkang What do you think?

jeehoonkang

Thank you for reporting and fixing a nasty bug! It's also a nice cleanup.

I also agree with @oliver-giersch in that maybe Arc<Mutex<T>> here is an overkill, but we can discuss optimizations later.

jeehoonkang · 2018-07-27T14:45:30Z

src/thread.rs

@@ -380,3 +376,5 @@ impl<'a> Drop for Scope<'a> {
        self.drop_all()
    }
 }
+
+type ScopedThreadResult<T> = Arc<Mutex<Option<T>>>;


Maybe too pedantic to say, but a newline here...? :)

ghost · 2018-07-27T15:15:47Z

This will need a rebase before merging.

oliver-giersch · 2018-07-27T21:37:52Z

I will merge the current master into the branch tomorrow and resolve the conflicts.

I've noticed that relying on safe code does make it significantly easier to reason about things. I think I have found an unsafe version now that works as well and is reasonably easy to understand, so I might make a PR for that as well.
However, unless this brings significant performance improvements it would be hard to argue for replacing perfectly fine safe code with a slightly better performing but much more fragile version.
Perhaps I could try to design some benchmarks to clarify things.

~~By the way I have done some tests and I am pretty sure that in the current implementation this code leaks memory for the Foo struct. With the Arc<Mutex<_>> this should not be possible.~~

Scratch that. The original code is a bit contrived but it is well thought through and actually prevents a memory leak in this case.

struct Foo([u8; 4096]);

#[test]
fn leak() {
    scope(|scope| {
        let guard = scope.spawn(|| {
            panic!();
            Foo([0; 4096])
        });
    });
}
```~~

# Conflicts: # src/thread.rs

…' into fix_issue_34_undefined_behaviour # Conflicts: # src/thread.rs

jeehoonkang · 2018-07-29T01:42:25Z

src/thread.rs

@@ -415,3 +411,5 @@ impl<'env> Drop for Scope<'env> {
        self.drop_all().unwrap();
    }
 }
+
+type ScopedThreadResult<T> = Arc<Mutex<Option<T>>>;


After inserting a newline here, I think we're ready to merge this PR. (Perhaps we can deal with the newline issue in another PR, too.) @oliver-giersch @stjepang are you okay with merging?

implements a more idiomatic way to share a spawned thread's result po…

50737a6

…inter between the scoped thread and the join handles, that avoids use of mem::ManuallyForget and mem::uninitialized().

oliver-giersch mentioned this pull request Jul 26, 2018

Remove the call to mem::uninitialized in scoped threads #34

Closed

ghost suggested changes Jul 26, 2018

View reviewed changes

replaces unsound ScopedThreadResult implementation (must not implemen…

7722304

…t Drop) in favor of a safe solution using std::sync::Arc and std::sync::Mutex

ghost approved these changes Jul 27, 2018

View reviewed changes

jeehoonkang approved these changes Jul 27, 2018

View reviewed changes

oliver-giersch added 2 commits July 28, 2018 13:24

Merge branch 'master' into fix_issue_34_undefined_behaviour

d8e89c3

# Conflicts: # src/thread.rs

Merge remote-tracking branch 'origin/fix_issue_34_undefined_behaviour…

d8eaa07

…' into fix_issue_34_undefined_behaviour # Conflicts: # src/thread.rs

jeehoonkang reviewed Jul 29, 2018

View reviewed changes

Add a newline at the end of file.

e52eb8b

ghost merged commit 5380e22 into crossbeam-rs:master Jul 29, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements a more idiomatic thread result without using mem::uninitialized() and the associated undefined behaviour #35

Implements a more idiomatic thread result without using mem::uninitialized() and the associated undefined behaviour #35

oliver-giersch commented Jul 26, 2018

oliver-giersch commented Jul 26, 2018 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

ghost Jul 26, 2018

ghost Jul 26, 2018

oliver-giersch Jul 27, 2018

ghost Jul 26, 2018

oliver-giersch Jul 27, 2018

ghost Jul 26, 2018

oliver-giersch Jul 27, 2018

oliver-giersch Jul 27, 2018

oliver-giersch commented Jul 27, 2018

ghost left a comment

jeehoonkang left a comment

jeehoonkang Jul 27, 2018

ghost commented Jul 27, 2018

oliver-giersch commented Jul 27, 2018 •

edited

Loading

jeehoonkang Jul 29, 2018

ghost Jul 29, 2018

Implements a more idiomatic thread result without using mem::uninitialized() and the associated undefined behaviour #35

Implements a more idiomatic thread result without using mem::uninitialized() and the associated undefined behaviour #35

Conversation

oliver-giersch commented Jul 26, 2018

oliver-giersch commented Jul 26, 2018 • edited Loading

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-giersch commented Jul 27, 2018

ghost left a comment

Choose a reason for hiding this comment

jeehoonkang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 27, 2018

oliver-giersch commented Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-giersch commented Jul 26, 2018 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

oliver-giersch commented Jul 27, 2018 •

edited

Loading