From b22861d16d6eae64134824485cc393ff0ee90c6a Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Fri, 30 Aug 2013 20:52:19 +0200 Subject: [PATCH 1/4] Rename MutexArc access methods to unsafe_access --- src/libextra/arc.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libextra/arc.rs b/src/libextra/arc.rs index 2ccbe396f8fd..1021722d85ed 100644 --- a/src/libextra/arc.rs +++ b/src/libextra/arc.rs @@ -214,7 +214,7 @@ impl MutexArc { * blocked on the mutex) will also fail immediately. */ #[inline] - pub unsafe fn access(&self, blk: &fn(x: &mut T) -> U) -> U { + pub unsafe fn unsafe_access(&self, blk: &fn(x: &mut T) -> U) -> U { let state = self.x.get(); // Borrowck would complain about this if the function were // not already unsafe. See borrow_rwlock, far below. @@ -227,7 +227,7 @@ impl MutexArc { /// As access(), but with a condvar, as sync::mutex.lock_cond(). #[inline] - pub unsafe fn access_cond<'x, 'c, U>(&self, + pub unsafe fn unsafe_access_cond<'x, 'c, U>(&self, blk: &fn(x: &'x mut T, c: &'c Condvar) -> U) -> U { @@ -597,12 +597,12 @@ mod tests { do task::spawn { // wait until parent gets in p.take().recv(); - do arc2.access_cond |state, cond| { + do arc2.unsafe_access_cond |state, cond| { *state = true; cond.signal(); } } - do arc.access_cond |state, cond| { + do arc.unsafe_access_cond |state, cond| { c.take().send(()); assert!(!*state); while !*state { @@ -621,14 +621,14 @@ mod tests { do task::spawn_unlinked { let _ = p.recv(); - do arc2.access_cond |one, cond| { + do arc2.unsafe_access_cond |one, cond| { cond.signal(); // Parent should fail when it wakes up. assert_eq!(*one, 0); } } - do arc.access_cond |one, cond| { + do arc.unsafe_access_cond |one, cond| { c.send(()); while *one == 1 { cond.wait(); @@ -642,11 +642,11 @@ mod tests { let arc = MutexArc::new(1); let arc2 = arc.clone(); do task::try { - do arc2.access |one| { + do arc2.unsafe_access |one| { assert_eq!(*one, 2); } }; - do arc.access |one| { + do arc.unsafe_access |one| { assert_eq!(*one, 1); } } @@ -658,7 +658,7 @@ mod tests { let (p, c) = comm::stream(); do task::spawn { unsafe { - do arc2.access |one| { + do arc2.unsafe_access |one| { c.send(()); assert!(*one == 2); } From 408367ba6dad7c687a96c1db47a41851f778f93c Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Wed, 4 Sep 2013 01:07:11 +0200 Subject: [PATCH 2/4] Add a safe implementation of MutexArc::access* methods Current access methods are nestable and unsafe. This patch renames current methods implementation - prepends unsafe_ - and implements 2 new methods that are both safe and un-nestable. Fixes #7473 --- src/libextra/arc.rs | 183 +++++++++++++++++++--- src/test/compile-fail/mutex-arc-nested.rs | 26 +++ 2 files changed, 188 insertions(+), 21 deletions(-) create mode 100644 src/test/compile-fail/mutex-arc-nested.rs diff --git a/src/libextra/arc.rs b/src/libextra/arc.rs index 1021722d85ed..2eaf44950c09 100644 --- a/src/libextra/arc.rs +++ b/src/libextra/arc.rs @@ -159,7 +159,9 @@ impl Clone for Arc { #[doc(hidden)] struct MutexArcInner { priv lock: Mutex, priv failed: bool, priv data: T } + /// An Arc with mutable data protected by a blocking mutex. +#[no_freeze] struct MutexArc { priv x: UnsafeArc> } @@ -190,6 +192,35 @@ impl MutexArc { MutexArc { x: UnsafeArc::new(data) } } + + /// Refer unsafe_access and access methods for the documentaiton. + #[inline] + unsafe fn lock_and_access(&self, blk: &fn(x: &mut T) -> U) -> U { + let state = self.x.get(); + // Borrowck would complain about this if the function were + // not already unsafe. See borrow_rwlock, far below. + do (&(*state).lock).lock { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data) + } + } + + #[inline] + unsafe fn lock_and_access_cond<'x, 'c, U>(&self, + blk: &fn(x: &'x mut T, + c: &'c Condvar) -> U) + -> U { + let state = self.x.get(); + do (&(*state).lock).lock_cond |cond| { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data, + &Condvar {is_mutex: true, + failed: &mut (*state).failed, + cond: cond }) + } + } /** * Access the underlying mutable data with mutual exclusion from other * tasks. The argument closure will be run with the mutex locked; all @@ -215,31 +246,16 @@ impl MutexArc { */ #[inline] pub unsafe fn unsafe_access(&self, blk: &fn(x: &mut T) -> U) -> U { - let state = self.x.get(); - // Borrowck would complain about this if the function were - // not already unsafe. See borrow_rwlock, far below. - do (&(*state).lock).lock { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data) - } + self.lock_and_access(blk) } - /// As access(), but with a condvar, as sync::mutex.lock_cond(). + /// As unsafe_access(), but with a condvar, as sync::mutex.lock_cond(). #[inline] pub unsafe fn unsafe_access_cond<'x, 'c, U>(&self, blk: &fn(x: &'x mut T, c: &'c Condvar) -> U) -> U { - let state = self.x.get(); - do (&(*state).lock).lock_cond |cond| { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data, - &Condvar {is_mutex: true, - failed: &mut (*state).failed, - cond: cond }) - } + self.lock_and_access_cond(blk) } /** @@ -259,6 +275,34 @@ impl MutexArc { } } +impl MutexArc { + + /** + * As unsafe_access. + * + * The difference between access and unsafe_access is that the former + * forbids mutexes to be nested. The purpose of this is to offer a safe + * implementation of both methods access and access_cond to be used instead + * of rwlock in cases where no readers are needed and sightly better performance + * is required. + * + * Both methods have the same failure behaviour as unsafe_access and + * unsafe_access_cond. + */ + #[inline] + pub fn access(&self, blk: &fn(x: &mut T) -> U) -> U { + unsafe { self.lock_and_access(blk) } + } + + #[inline] + pub fn access_cond<'x, 'c, U>(&self, + blk: &fn(x: &'x mut T, + c: &'c Condvar) -> U) + -> U { + unsafe { self.lock_and_access_cond(blk) } + } +} + // Common code for {mutex.access,rwlock.write}{,_cond}. #[inline] #[doc(hidden)] @@ -589,6 +633,100 @@ mod tests { #[test] fn test_mutex_arc_condvar() { + let arc = ~MutexArc::new(false); + let arc2 = ~arc.clone(); + let (p,c) = comm::oneshot(); + let (c,p) = (Cell::new(c), Cell::new(p)); + do task::spawn || { + // wait until parent gets in + p.take().recv(); + do arc2.access_cond |state, cond| { + *state = true; + cond.signal(); + } + } + + do arc.access_cond |state, cond| { + c.take().send(()); + assert!(!*state); + while !*state { + cond.wait(); + } + } + } + + #[test] #[should_fail] + fn test_arc_condvar_poison() { + let arc = ~MutexArc::new(1); + let arc2 = ~arc.clone(); + let (p, c) = comm::stream(); + + do task::spawn_unlinked || { + let _ = p.recv(); + do arc2.access_cond |one, cond| { + cond.signal(); + // Parent should fail when it wakes up. + assert_eq!(*one, 0); + } + } + + do arc.access_cond |one, cond| { + c.send(()); + while *one == 1 { + cond.wait(); + } + } + } + + #[test] #[should_fail] + fn test_mutex_arc_poison() { + let arc = ~MutexArc::new(1); + let arc2 = ~arc.clone(); + do task::try || { + do arc2.access |one| { + assert_eq!(*one, 2); + } + }; + do arc.access |one| { + assert_eq!(*one, 1); + } + } + + #[test] #[should_fail] + pub fn test_mutex_arc_unwrap_poison() { + let arc = MutexArc::new(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 = arc.unwrap(); + assert!(one == 1); + } + + #[test] + fn test_unsafe_mutex_arc_nested() { + unsafe { + // Tests nested mutexes and access + // to underlaying data. + let arc = ~MutexArc::new(1); + let arc2 = ~MutexArc::new(*arc); + do task::spawn || { + do (*arc2).unsafe_access |mutex| { + do (*mutex).access |one| { + assert!(*one == 1); + } + } + }; + } + } + + #[test] + fn test_unsafe_mutex_arc_condvar() { unsafe { let arc = MutexArc::new(false); let arc2 = arc.clone(); @@ -613,7 +751,7 @@ mod tests { } #[test] #[should_fail] - fn test_arc_condvar_poison() { + fn test_unsafe_arc_condvar_poison() { unsafe { let arc = MutexArc::new(1); let arc2 = arc.clone(); @@ -637,7 +775,7 @@ mod tests { } } #[test] #[should_fail] - fn test_mutex_arc_poison() { + fn test_unsafe_mutex_arc_poison() { unsafe { let arc = MutexArc::new(1); let arc2 = arc.clone(); @@ -651,8 +789,9 @@ mod tests { } } } + #[test] #[should_fail] - pub fn test_mutex_arc_unwrap_poison() { + pub fn test_unsafe_mutex_arc_unwrap_poison() { let arc = MutexArc::new(1); let arc2 = arc.clone(); let (p, c) = comm::stream(); @@ -668,6 +807,7 @@ mod tests { let one = arc.unwrap(); assert!(one == 1); } + #[test] #[should_fail] fn test_rw_arc_poison_wr() { let arc = RWArc::new(1); @@ -681,6 +821,7 @@ mod tests { assert_eq!(*one, 1); } } + #[test] #[should_fail] fn test_rw_arc_poison_ww() { let arc = RWArc::new(1); diff --git a/src/test/compile-fail/mutex-arc-nested.rs b/src/test/compile-fail/mutex-arc-nested.rs new file mode 100644 index 000000000000..e5f2a2fbbe10 --- /dev/null +++ b/src/test/compile-fail/mutex-arc-nested.rs @@ -0,0 +1,26 @@ +// Copyright 2013 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +extern mod extra; + +use std::task; +use extra::arc::{MutexArc}; + +fn test_mutex_arc_nested() { + let arc = ~MutexArc::new(1); + let arc2 = ~MutexArc::new(*arc); + + do task::spawn || { + do (*arc2).access |mutex| { // This should fail because MutexArc is not Freeze + } + }; +} + +fn main() {} From c0aa62c872c745ce3a13f60da199752b035f9c48 Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Wed, 4 Sep 2013 02:24:04 +0200 Subject: [PATCH 3/4] Fixed docs and styles --- src/libextra/arc.rs | 152 +++++----------------- src/test/compile-fail/mutex-arc-nested.rs | 2 +- 2 files changed, 31 insertions(+), 123 deletions(-) diff --git a/src/libextra/arc.rs b/src/libextra/arc.rs index 2eaf44950c09..08f57764e59b 100644 --- a/src/libextra/arc.rs +++ b/src/libextra/arc.rs @@ -192,35 +192,6 @@ impl MutexArc { MutexArc { x: UnsafeArc::new(data) } } - - /// Refer unsafe_access and access methods for the documentaiton. - #[inline] - unsafe fn lock_and_access(&self, blk: &fn(x: &mut T) -> U) -> U { - let state = self.x.get(); - // Borrowck would complain about this if the function were - // not already unsafe. See borrow_rwlock, far below. - do (&(*state).lock).lock { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data) - } - } - - #[inline] - unsafe fn lock_and_access_cond<'x, 'c, U>(&self, - blk: &fn(x: &'x mut T, - c: &'c Condvar) -> U) - -> U { - let state = self.x.get(); - do (&(*state).lock).lock_cond |cond| { - check_poison(true, (*state).failed); - let _z = PoisonOnFail(&mut (*state).failed); - blk(&mut (*state).data, - &Condvar {is_mutex: true, - failed: &mut (*state).failed, - cond: cond }) - } - } /** * Access the underlying mutable data with mutual exclusion from other * tasks. The argument closure will be run with the mutex locked; all @@ -246,7 +217,14 @@ impl MutexArc { */ #[inline] pub unsafe fn unsafe_access(&self, blk: &fn(x: &mut T) -> U) -> U { - self.lock_and_access(blk) + let state = self.x.get(); + // Borrowck would complain about this if the function were + // not already unsafe. See borrow_rwlock, far below. + do (&(*state).lock).lock { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data) + } } /// As unsafe_access(), but with a condvar, as sync::mutex.lock_cond(). @@ -255,7 +233,15 @@ impl MutexArc { blk: &fn(x: &'x mut T, c: &'c Condvar) -> U) -> U { - self.lock_and_access_cond(blk) + let state = self.x.get(); + do (&(*state).lock).lock_cond |cond| { + check_poison(true, (*state).failed); + let _z = PoisonOnFail(&mut (*state).failed); + blk(&mut (*state).data, + &Condvar {is_mutex: true, + failed: &mut (*state).failed, + cond: cond }) + } } /** @@ -281,25 +267,30 @@ impl MutexArc { * As unsafe_access. * * The difference between access and unsafe_access is that the former - * forbids mutexes to be nested. The purpose of this is to offer a safe - * implementation of both methods access and access_cond to be used instead - * of rwlock in cases where no readers are needed and sightly better performance - * is required. + * forbids mutexes to be nested. While unsafe_access can be used on + * MutexArcs without freezable interiors, this safe version of access + * requires the Freeze bound, which prohibits access on MutexArcs which + * might contain nested MutexArcs inside. + * + * The purpose of this is to offer a safe implementation of both methods + * access and access_cond to be used instead of rwlock in cases where no + * readers are needed and sightly better performance is required. * * Both methods have the same failure behaviour as unsafe_access and * unsafe_access_cond. */ #[inline] pub fn access(&self, blk: &fn(x: &mut T) -> U) -> U { - unsafe { self.lock_and_access(blk) } + unsafe { self.unsafe_access(blk) } } - + + /// As unsafe_access_cond but safe and Freeze. #[inline] pub fn access_cond<'x, 'c, U>(&self, blk: &fn(x: &'x mut T, c: &'c Condvar) -> U) -> U { - unsafe { self.lock_and_access_cond(blk) } + unsafe { self.unsafe_access_cond(blk) } } } @@ -707,7 +698,7 @@ mod tests { let one = arc.unwrap(); assert!(one == 1); } - + #[test] fn test_unsafe_mutex_arc_nested() { unsafe { @@ -722,92 +713,9 @@ mod tests { } } }; - } - } - - #[test] - fn test_unsafe_mutex_arc_condvar() { - unsafe { - let arc = MutexArc::new(false); - let arc2 = arc.clone(); - let (p, c) = comm::oneshot(); - let (c, p) = (Cell::new(c), Cell::new(p)); - do task::spawn { - // wait until parent gets in - p.take().recv(); - do arc2.unsafe_access_cond |state, cond| { - *state = true; - cond.signal(); - } - } - do arc.unsafe_access_cond |state, cond| { - c.take().send(()); - assert!(!*state); - while !*state { - cond.wait(); - } - } } } - #[test] #[should_fail] - fn test_unsafe_arc_condvar_poison() { - unsafe { - let arc = MutexArc::new(1); - let arc2 = arc.clone(); - let (p, c) = comm::stream(); - - do task::spawn_unlinked { - let _ = p.recv(); - do arc2.unsafe_access_cond |one, cond| { - cond.signal(); - // Parent should fail when it wakes up. - assert_eq!(*one, 0); - } - } - - do arc.unsafe_access_cond |one, cond| { - c.send(()); - while *one == 1 { - cond.wait(); - } - } - } - } - #[test] #[should_fail] - fn test_unsafe_mutex_arc_poison() { - unsafe { - let arc = MutexArc::new(1); - let arc2 = arc.clone(); - do task::try { - do arc2.unsafe_access |one| { - assert_eq!(*one, 2); - } - }; - do arc.unsafe_access |one| { - assert_eq!(*one, 1); - } - } - } - - #[test] #[should_fail] - pub fn test_unsafe_mutex_arc_unwrap_poison() { - let arc = MutexArc::new(1); - let arc2 = arc.clone(); - let (p, c) = comm::stream(); - do task::spawn { - unsafe { - do arc2.unsafe_access |one| { - c.send(()); - assert!(*one == 2); - } - } - } - let _ = p.recv(); - let one = arc.unwrap(); - assert!(one == 1); - } - #[test] #[should_fail] fn test_rw_arc_poison_wr() { let arc = RWArc::new(1); diff --git a/src/test/compile-fail/mutex-arc-nested.rs b/src/test/compile-fail/mutex-arc-nested.rs index e5f2a2fbbe10..bac17dec8ecb 100644 --- a/src/test/compile-fail/mutex-arc-nested.rs +++ b/src/test/compile-fail/mutex-arc-nested.rs @@ -18,7 +18,7 @@ fn test_mutex_arc_nested() { let arc2 = ~MutexArc::new(*arc); do task::spawn || { - do (*arc2).access |mutex| { // This should fail because MutexArc is not Freeze + do (*arc2).access |mutex| { //~ ERROR instantiating a type parameter with an incompatible type } }; } From d0ad2513766733abc99da6af0cfe130abcd70744 Mon Sep 17 00:00:00 2001 From: Flaper Fesp Date: Wed, 4 Sep 2013 09:14:56 +0200 Subject: [PATCH 4/4] Use MuextArc and RWArc in docstrings --- src/libextra/arc.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libextra/arc.rs b/src/libextra/arc.rs index 08f57764e59b..3fbfae52c630 100644 --- a/src/libextra/arc.rs +++ b/src/libextra/arc.rs @@ -201,10 +201,10 @@ impl MutexArc { * The reason this function is 'unsafe' is because it is possible to * construct a circular reference among multiple Arcs by mutating the * underlying data. This creates potential for deadlock, but worse, this - * will guarantee a memory leak of all involved Arcs. Using mutex Arcs + * will guarantee a memory leak of all involved Arcs. Using MutexArcs * inside of other Arcs is safe in absence of circular references. * - * If you wish to nest mutex_arcs, one strategy for ensuring safety at + * If you wish to nest MutexArcs, one strategy for ensuring safety at * runtime is to add a "nesting level counter" inside the stored data, and * when traversing the arcs, assert that they monotonically decrease. * @@ -272,9 +272,9 @@ impl MutexArc { * requires the Freeze bound, which prohibits access on MutexArcs which * might contain nested MutexArcs inside. * - * The purpose of this is to offer a safe implementation of both methods - * access and access_cond to be used instead of rwlock in cases where no - * readers are needed and sightly better performance is required. + * The purpose of this is to offer a safe implementation of MutexArc to be + * used instead of RWArc in cases where no readers are needed and sightly + * better performance is required. * * Both methods have the same failure behaviour as unsafe_access and * unsafe_access_cond.