auto merge of #5176 : brson/rust/unwrap_shared_mutable_state, r=nikomatsakis

r?

This fixes the current [random failures](http://buildbot.rust-lang.org/builders/auto-linux/builds/291/steps/test/logs/stdio) on the bots and closes #4436 by removing `unwrap_shared_mutable_state` and the code that depends on it. The result is that ARC-like things will not be unwrappable. This feature is complex and is not used outside of test cases.

Note that there is not consensus to remove it.

(second commit)

Author: bors

Diff for: src/libcore/private.rs

@@ -107,20 +107,9 @@ fn compare_and_swap(address: &mut int, oldval: int, newval: int) -> bool {
  * Shared state & exclusive ARC
  ****************************************************************************/
 
-struct UnwrapProtoInner {
-    contents: Option<(comm::ChanOne<()>,  comm::PortOne<bool>)>,
-}
-
-// An unwrapper uses this protocol to communicate with the "other" task that
-// drops the last refcount on an arc. Unfortunately this can't be a proper
-// pipe protocol because the unwrapper has to access both stages at once.
-type UnwrapProto = ~UnwrapProtoInner;
-
 struct ArcData<T> {
     mut count:     libc::intptr_t,
-    mut unwrapper: int, // either a UnwrapProto or 0
-    // FIXME(#3224) should be able to make this non-option to save memory, and
-    // in unwrap() use "let ~ArcData { data: result, _ } = thing" to unwrap it
+    // FIXME(#3224) should be able to make this non-option to save memory
     mut data:      Option<T>,
 }
 
@@ -131,37 +120,13 @@ struct ArcDestruct<T> {
 impl<T> Drop for ArcDestruct<T>{
     fn finalize(&self) {
         unsafe {
-            if self.data.is_null() {
-                return; // Happens when destructing an unwrapper's handle.
-            }
             do task::unkillable {
                 let data: ~ArcData<T> = cast::reinterpret_cast(&self.data);
                 let new_count =
                     intrinsics::atomic_xsub(&mut data.count, 1) - 1;
                 assert new_count >= 0;
                 if new_count == 0 {
-                    // Were we really last, or should we hand off to an
-                    // unwrapper? It's safe to not xchg because the unwrapper
-                    // will set the unwrap lock *before* dropping his/her
-                    // reference. In effect, being here means we're the only
-                    // *awake* task with the data.
-                    if data.unwrapper != 0 {
-                        let mut p: UnwrapProto =
-                            cast::reinterpret_cast(&data.unwrapper);
-                        let (message, response) =
-                            option::swap_unwrap(&mut p.contents);
-                        // Send 'ready' and wait for a response.
-                        comm::send_one(message, ());
-                        // Unkillable wait. Message guaranteed to come.
-                        if comm::recv_one(response) {
-                            // Other task got the data.
-                            cast::forget(data);
-                        } else {
-                            // Other task was killed. drop glue takes over.
-                        }
-                    } else {
-                        // drop glue takes over.
-                    }
+                    // drop glue takes over.
                 } else {
                     cast::forget(data);
                 }
@@ -176,79 +141,6 @@ fn ArcDestruct<T>(data: *libc::c_void) -> ArcDestruct<T> {
     }
 }
 
-pub unsafe fn unwrap_shared_mutable_state<T:Owned>(rc: SharedMutableState<T>)
-        -> T {
-    struct DeathThroes<T> {
-        mut ptr:      Option<~ArcData<T>>,
-        mut response: Option<comm::ChanOne<bool>>,
-    }
-
-    impl<T> Drop for DeathThroes<T>{
-        fn finalize(&self) {
-            unsafe {
-                let response = option::swap_unwrap(&mut self.response);
-                // In case we get killed early, we need to tell the person who
-                // tried to wake us whether they should hand-off the data to
-                // us.
-                if task::failing() {
-                    comm::send_one(response, false);
-                    // Either this swap_unwrap or the one below (at "Got
-                    // here") ought to run.
-                    cast::forget(option::swap_unwrap(&mut self.ptr));
-                } else {
-                    assert self.ptr.is_none();
-                    comm::send_one(response, true);
-                }
-            }
-        }
-    }
-
-    do task::unkillable {
-        let ptr: ~ArcData<T> = cast::reinterpret_cast(&rc.data);
-        let (p1,c1) = comm::oneshot(); // ()
-        let (p2,c2) = comm::oneshot(); // bool
-        let mut server: UnwrapProto = ~UnwrapProtoInner {
-            contents: Some((c1,p2))
-        };
-        let serverp: int = cast::transmute(server);
-        // Try to put our server end in the unwrapper slot.
-        if compare_and_swap(&mut ptr.unwrapper, 0, serverp) {
-            // Got in. Step 0: Tell destructor not to run. We are now it.
-            rc.data = ptr::null();
-            // Step 1 - drop our own reference.
-            let new_count = intrinsics::atomic_xsub(&mut ptr.count, 1) - 1;
-            //assert new_count >= 0;
-            if new_count == 0 {
-                // We were the last owner. Can unwrap immediately.
-                // Also we have to free the server endpoints.
-                let _server: UnwrapProto = cast::transmute(serverp);
-                option::swap_unwrap(&mut ptr.data)
-                // drop glue takes over.
-            } else {
-                // The *next* person who sees the refcount hit 0 will wake us.
-                let end_result =
-                    DeathThroes { ptr: Some(ptr),
-                                  response: Some(c2) };
-                let mut p1 = Some(p1); // argh
-                do task::rekillable {
-                    comm::recv_one(option::swap_unwrap(&mut p1));
-                }
-                // Got here. Back in the 'unkillable' without getting killed.
-                // Recover ownership of ptr, then take the data out.
-                let ptr = option::swap_unwrap(&mut end_result.ptr);
-                option::swap_unwrap(&mut ptr.data)
-                // drop glue takes over.
-            }
-        } else {
-            // Somebody else was trying to unwrap. Avoid guaranteed deadlock.
-            cast::forget(ptr);
-            // Also we have to free the (rejected) server endpoints.
-            let _server: UnwrapProto = cast::transmute(serverp);
-            fail!(~"Another task is already unwrapping this ARC!");
-        }
-    }
-}
-
 /**
  * COMPLETELY UNSAFE. Used as a primitive for the safe versions in std::arc.
  *
@@ -259,7 +151,7 @@ pub type SharedMutableState<T> = ArcDestruct<T>;
 
 pub unsafe fn shared_mutable_state<T:Owned>(data: T) ->
         SharedMutableState<T> {
-    let data = ~ArcData { count: 1, unwrapper: 0, data: Some(data) };
+    let data = ~ArcData { count: 1, data: Some(data) };
     unsafe {
         let ptr = cast::transmute(data);
         ArcDestruct(ptr)
@@ -413,22 +305,14 @@ pub impl<T:Owned> Exclusive<T> {
     }
 }
 
-// FIXME(#3724) make this a by-move method on the exclusive
-pub fn unwrap_exclusive<T:Owned>(arc: Exclusive<T>) -> T {
-    let Exclusive { x: x } = arc;
-    let inner = unsafe { unwrap_shared_mutable_state(x) };
-    let ExData { data: data, _ } = inner;
-    data
-}
-
 #[cfg(test)]
 pub mod tests {
     use core::option::{None, Some};
 
     use cell::Cell;
     use comm;
     use option;
-    use private::{exclusive, unwrap_exclusive};
+    use private::exclusive;
     use result;
     use task;
     use uint;
@@ -479,70 +363,4 @@ pub mod tests {
             assert *one == 1;
         }
     }
-
-    #[test]
-    pub fn exclusive_unwrap_basic() {
-        let x = exclusive(~~"hello");
-        assert unwrap_exclusive(x) == ~~"hello";
-    }
-
-    #[test]
-    pub fn exclusive_unwrap_contended() {
-        let x = exclusive(~~"hello");
-        let x2 = Cell(x.clone());
-        do task::spawn {
-            let x2 = x2.take();
-            do x2.with |_hello| { }
-            task::yield();
-        }
-        assert unwrap_exclusive(x) == ~~"hello";
-
-        // Now try the same thing, but with the child task blocking.
-        let x = exclusive(~~"hello");
-        let x2 = Cell(x.clone());
-        let mut res = None;
-        do task::task().future_result(|+r| res = Some(r)).spawn {
-            let x2 = x2.take();
-            assert unwrap_exclusive(x2) == ~~"hello";
-        }
-        // Have to get rid of our reference before blocking.
-        { let _x = x; } // FIXME(#3161) util::ignore doesn't work here
-        let res = option::swap_unwrap(&mut res);
-        res.recv();
-    }
-
-    #[test] #[should_fail] #[ignore(cfg(windows))]
-    pub fn exclusive_unwrap_conflict() {
-        let x = exclusive(~~"hello");
-        let x2 = Cell(x.clone());
-        let mut res = None;
-        do task::task().future_result(|+r| res = Some(r)).spawn {
-            let x2 = x2.take();
-            assert unwrap_exclusive(x2) == ~~"hello";
-        }
-        assert unwrap_exclusive(x) == ~~"hello";
-        let res = option::swap_unwrap(&mut res);
-        // See #4689 for why this can't be just "res.recv()".
-        assert res.recv() == task::Success;
-    }
-
-    #[test] #[ignore(cfg(windows))]
-    pub fn exclusive_unwrap_deadlock() {
-        // This is not guaranteed to get to the deadlock before being killed,
-        // but it will show up sometimes, and if the deadlock were not there,
-        // the test would nondeterministically fail.
-        let result = do task::try {
-            // a task that has two references to the same exclusive will
-            // deadlock when it unwraps. nothing to be done about that.
-            let x = exclusive(~~"hello");
-            let x2 = x.clone();
-            do task::spawn {
-                for 10.times { task::yield(); } // try to let the unwrapper go
-                fail!(); // punt it awake from its deadlock
-            }
-            let _z = unwrap_exclusive(x);
-            do x2.with |_hello| { }
-        };
-        assert result.is_err();
-    }
 }

Diff for: src/libstd/arc.rs

@@ -21,7 +21,7 @@ use core::cell::Cell;
 use core::pipes;
 use core::prelude::*;
 use core::private::{SharedMutableState, shared_mutable_state};
-use core::private::{clone_shared_mutable_state, unwrap_shared_mutable_state};
+use core::private::{clone_shared_mutable_state};
 use core::private::{get_shared_mutable_state, get_shared_immutable_state};
 use core::ptr;
 use core::task;
@@ -104,20 +104,6 @@ pub fn clone<T:Const + Owned>(rc: &ARC<T>) -> ARC<T> {
     ARC { x: unsafe { clone_shared_mutable_state(&rc.x) } }
 }
 
-/**
- * Retrieve the data back out of the ARC. This function blocks until the
- * reference given to it is the last existing one, and then unwrap the data
- * instead of destroying it.
- *
- * If multiple tasks call unwrap, all but the first will fail. Do not call
- * unwrap from a task that holds another reference to the same ARC; it is
- * guaranteed to deadlock.
- */
-pub fn unwrap<T:Const + Owned>(rc: ARC<T>) -> T {
-    let ARC { x: x } = rc;
-    unsafe { unwrap_shared_mutable_state(x) }
-}
-
 impl<T:Const + Owned> Clone for ARC<T> {
     fn clone(&self) -> ARC<T> {
         clone(self)
@@ -213,23 +199,6 @@ pub impl<T:Owned> &MutexARC<T> {
     }
 }
 
-/**
- * Retrieves the data, blocking until all other references are dropped,
- * exactly as arc::unwrap.
- *
- * Will additionally fail if another task has failed while accessing the arc.
- */
-// FIXME(#3724) make this a by-move method on the arc
-pub fn unwrap_mutex_arc<T:Owned>(arc: MutexARC<T>) -> T {
-    let MutexARC { x: x } = arc;
-    let inner = unsafe { unwrap_shared_mutable_state(x) };
-    let MutexARCInner { failed: failed, data: data, _ } = inner;
-    if failed {
-        fail!(~"Can't unwrap poisoned MutexARC - another task failed inside!")
-    }
-    data
-}
-
 // Common code for {mutex.access,rwlock.write}{,_cond}.
 #[inline(always)]
 #[doc(hidden)]
@@ -411,24 +380,6 @@ pub impl<T:Const + Owned> &RWARC<T> {
     }
 }
 
-/**
- * Retrieves the data, blocking until all other references are dropped,
- * exactly as arc::unwrap.
- *
- * Will additionally fail if another task has failed while accessing the arc
- * in write mode.
- */
-// FIXME(#3724) make this a by-move method on the arc
-pub fn unwrap_rw_arc<T:Const + Owned>(arc: RWARC<T>) -> T {
-    let RWARC { x: x, _ } = arc;
-    let inner = unsafe { unwrap_shared_mutable_state(x) };
-    let RWARCInner { failed: failed, data: data, _ } = inner;
-    if failed {
-        fail!(~"Can't unwrap poisoned RWARC - another task failed inside!")
-    }
-    data
-}
-
 // Borrowck rightly complains about immutably aliasing the rwlock in order to
 // lock it. This wraps the unsafety, with the justification that the 'lock'
 // field is never overwritten; only 'failed' and 'data'.
@@ -586,21 +537,6 @@ mod tests {
         }
     }
     #[test] #[should_fail] #[ignore(cfg(windows))]
-    pub fn test_mutex_arc_unwrap_poison() {
-        let arc = MutexARC(1);
-        let arc2 = ~(&arc).clone();
-        let (p, c) = comm::stream();
-        do task::spawn || {
-            do arc2.access |one| {
-                c.send(());
-                assert *one == 2;
-            }
-        }
-        let _ = p.recv();
-        let one = unwrap_mutex_arc(arc);
-        assert one == 1;
-    }
-    #[test] #[should_fail] #[ignore(cfg(windows))]
     pub fn test_rw_arc_poison_wr() {
         let arc = ~RWARC(1);
         let arc2 = ~arc.clone();

Diff for: src/rt/rust_kernel.cpp

@@ -342,8 +342,11 @@ rust_kernel::register_exit_function(spawn_fn runner, fn_env_pair *f) {
     assert(!at_exit_started && "registering at_exit function after exit");
 
     if (at_exit_runner) {
-        assert(runner == at_exit_runner
-               && "there can be only one at_exit_runner");
+        // FIXME #2912 Would be very nice to assert this but we can't because
+        // of the way coretest works (the test case ends up using its own
+        // function)
+        //assert(runner == at_exit_runner
+        //       && "there can be only one at_exit_runner");
     }
 
     at_exit_runner = runner;