From 95c4899e55e7aab68f06e67660257d73e6a46eda Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 7 Jun 2020 23:36:07 +0200 Subject: [PATCH 1/7] Added an example where explicitly dropping a lock is necessary/a good idea. --- src/libstd/sync/mutex.rs | 61 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index 797b22fdd1279..6077e1a402965 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -107,6 +107,67 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// /// *guard += 1; /// ``` +/// +/// It is sometimes a good idea (or even necessary) to manually drop the mutex +/// to unlock it as soon as possible. If you need the resource until the end of +/// the scope, this is not needed. +/// +/// ``` +/// use std::sync::{Arc, Mutex}; +/// use std::thread; +/// +/// const N: usize = 3; +/// +/// // Some data to work with in multiple threads. +/// let data_mutex = Arc::new(Mutex::new([1, 2, 3, 4])); +/// // The result of all the work across all threads. +/// let res_mutex = Arc::new(Mutex::new(0)); +/// +/// // Threads other than the main thread. +/// let mut threads = Vec::with_capacity(N); +/// (0..N).for_each(|_| { +/// // Getting clones for the mutexes. +/// let data_mutex_clone = Arc::clone(&data_mutex); +/// let res_mutex_clone = Arc::clone(&res_mutex); +/// +/// threads.push(thread::spawn(move || { +/// let data = *data_mutex_clone.lock().unwrap(); +/// // This is the result of some important and long-ish work. +/// let result = data.iter().fold(0, |acc, x| acc + x * 2); +/// // We drop the `data` explicitely because it's not necessary anymore +/// // and the thread still has work to do. This allow other threads to +/// // start working on the data immediately, without waiting +/// // for the rest of the unrelated work to be done here. +/// std::mem::drop(data); +/// *res_mutex_clone.lock().unwrap() += result; +/// })); +/// }); +/// +/// let data = *data_mutex.lock().unwrap(); +/// // This is the result of some important and long-ish work. +/// let result = data.iter().fold(0, |acc, x| acc + x * 2); +/// // We drop the `data` explicitely because it's not necessary anymore +/// // and the thread still has work to do. This allow other threads to +/// // start working on the data immediately, without waiting +/// // for the rest of the unrelated work to be done here. +/// // +/// // It's even more important here because we `.join` the threads after that. +/// // If we had not dropped the lock, a thread could be waiting forever for +/// // it, causing a deadlock. +/// std::mem::drop(data); +/// // Here the lock is not assigned to a variable and so, even if the scope +/// // does not end after this line, the mutex is still released: +/// // there is no deadlock. +/// *res_mutex.lock().unwrap() += result; +/// +/// threads.into_iter().for_each(|thread| { +/// thread +/// .join() +/// .expect("The thread creating or execution failed !") +/// }); +/// +/// assert_eq!(*res_mutex.lock().unwrap(), 80); +/// ``` #[stable(feature = "rust1", since = "1.0.0")] #[cfg_attr(not(test), rustc_diagnostic_item = "mutex_type")] pub struct Mutex { From 9c8f881ccd050f06387612e4b8aa18111c51a63b Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Sun, 7 Jun 2020 23:42:55 +0200 Subject: [PATCH 2/7] Improved the example to work with mutable data, providing a reason for the mutex holding it --- src/libstd/sync/mutex.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index 6077e1a402965..633496154aefe 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -119,7 +119,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// const N: usize = 3; /// /// // Some data to work with in multiple threads. -/// let data_mutex = Arc::new(Mutex::new([1, 2, 3, 4])); +/// let data_mutex = Arc::new(Mutex::new(vec![1, 2, 3, 4])); /// // The result of all the work across all threads. /// let res_mutex = Arc::new(Mutex::new(0)); /// @@ -131,9 +131,10 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// let res_mutex_clone = Arc::clone(&res_mutex); /// /// threads.push(thread::spawn(move || { -/// let data = *data_mutex_clone.lock().unwrap(); +/// let mut data = data_mutex_clone.lock().unwrap(); /// // This is the result of some important and long-ish work. /// let result = data.iter().fold(0, |acc, x| acc + x * 2); +/// data.push(result); /// // We drop the `data` explicitely because it's not necessary anymore /// // and the thread still has work to do. This allow other threads to /// // start working on the data immediately, without waiting @@ -143,9 +144,10 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// })); /// }); /// -/// let data = *data_mutex.lock().unwrap(); +/// let mut data = data_mutex.lock().unwrap(); /// // This is the result of some important and long-ish work. /// let result = data.iter().fold(0, |acc, x| acc + x * 2); +/// data.push(result); /// // We drop the `data` explicitely because it's not necessary anymore /// // and the thread still has work to do. This allow other threads to /// // start working on the data immediately, without waiting @@ -166,7 +168,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// .expect("The thread creating or execution failed !") /// }); /// -/// assert_eq!(*res_mutex.lock().unwrap(), 80); +/// assert_eq!(*res_mutex.lock().unwrap(), 800); /// ``` #[stable(feature = "rust1", since = "1.0.0")] #[cfg_attr(not(test), rustc_diagnostic_item = "mutex_type")] From fdef1a5915332a3f9012b41f4176cf2a16025257 Mon Sep 17 00:00:00 2001 From: Poliorcetics Date: Mon, 8 Jun 2020 16:29:47 +0200 Subject: [PATCH 3/7] Simply use drop instead of std::mem::drop Co-authored-by: LeSeulArtichaut --- src/libstd/sync/mutex.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index 633496154aefe..c2c86fae654cf 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -139,7 +139,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// // and the thread still has work to do. This allow other threads to /// // start working on the data immediately, without waiting /// // for the rest of the unrelated work to be done here. -/// std::mem::drop(data); +/// drop(data); /// *res_mutex_clone.lock().unwrap() += result; /// })); /// }); @@ -156,7 +156,7 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// // It's even more important here because we `.join` the threads after that. /// // If we had not dropped the lock, a thread could be waiting forever for /// // it, causing a deadlock. -/// std::mem::drop(data); +/// drop(data); /// // Here the lock is not assigned to a variable and so, even if the scope /// // does not end after this line, the mutex is still released: /// // there is no deadlock. From 1312d30a6a837f72c3f36f5dc1c575a29890aa2c Mon Sep 17 00:00:00 2001 From: Alexis Bourget Date: Tue, 9 Jun 2020 22:40:30 +0200 Subject: [PATCH 4/7] Remove a lot of unecessary/duplicated comments --- src/libstd/sync/mutex.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index c2c86fae654cf..b3ef521d6ecb0 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -118,15 +118,11 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// /// const N: usize = 3; /// -/// // Some data to work with in multiple threads. /// let data_mutex = Arc::new(Mutex::new(vec![1, 2, 3, 4])); -/// // The result of all the work across all threads. /// let res_mutex = Arc::new(Mutex::new(0)); /// -/// // Threads other than the main thread. /// let mut threads = Vec::with_capacity(N); /// (0..N).for_each(|_| { -/// // Getting clones for the mutexes. /// let data_mutex_clone = Arc::clone(&data_mutex); /// let res_mutex_clone = Arc::clone(&res_mutex); /// @@ -135,10 +131,6 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// // This is the result of some important and long-ish work. /// let result = data.iter().fold(0, |acc, x| acc + x * 2); /// data.push(result); -/// // We drop the `data` explicitely because it's not necessary anymore -/// // and the thread still has work to do. This allow other threads to -/// // start working on the data immediately, without waiting -/// // for the rest of the unrelated work to be done here. /// drop(data); /// *res_mutex_clone.lock().unwrap() += result; /// })); @@ -153,9 +145,9 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// // start working on the data immediately, without waiting /// // for the rest of the unrelated work to be done here. /// // -/// // It's even more important here because we `.join` the threads after that. -/// // If we had not dropped the lock, a thread could be waiting forever for -/// // it, causing a deadlock. +/// // It's even more important here than in the threads because we `.join` the +/// // threads after that. If we had not dropped the lock, a thread could be +/// // waiting forever for it, causing a deadlock. /// drop(data); /// // Here the lock is not assigned to a variable and so, even if the scope /// // does not end after this line, the mutex is still released: From f747073fc1751afd2cfd4395283a4822b618f2da Mon Sep 17 00:00:00 2001 From: Poliorcetics Date: Sat, 13 Jun 2020 18:41:01 +0200 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: David Tolnay --- src/libstd/sync/mutex.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index b3ef521d6ecb0..6625d4659dcac 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -108,8 +108,8 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// *guard += 1; /// ``` /// -/// It is sometimes a good idea (or even necessary) to manually drop the mutex -/// to unlock it as soon as possible. If you need the resource until the end of +/// It is sometimes necessary to manually drop the mutex +/// guard to unlock it as soon as possible. If you need the resource until the end of /// the scope, this is not needed. /// /// ``` @@ -140,16 +140,16 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// // This is the result of some important and long-ish work. /// let result = data.iter().fold(0, |acc, x| acc + x * 2); /// data.push(result); -/// // We drop the `data` explicitely because it's not necessary anymore +/// // We drop the `data` explicitly because it's not necessary anymore /// // and the thread still has work to do. This allow other threads to /// // start working on the data immediately, without waiting /// // for the rest of the unrelated work to be done here. /// // /// // It's even more important here than in the threads because we `.join` the -/// // threads after that. If we had not dropped the lock, a thread could be +/// // threads after that. If we had not dropped the mutex guard, a thread could be /// // waiting forever for it, causing a deadlock. /// drop(data); -/// // Here the lock is not assigned to a variable and so, even if the scope +/// // Here the mutex guard is not assigned to a variable and so, even if the scope /// // does not end after this line, the mutex is still released: /// // there is no deadlock. /// *res_mutex.lock().unwrap() += result; From 34b3ff06e101f60cb69268ee83c93c177b63c322 Mon Sep 17 00:00:00 2001 From: Poliorcetics Date: Sat, 13 Jun 2020 18:43:37 +0200 Subject: [PATCH 6/7] Clarify the scope-related explanation Based on the review made by dtolnay. --- src/libstd/sync/mutex.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index 6625d4659dcac..37c8125b0984a 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -108,9 +108,8 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// *guard += 1; /// ``` /// -/// It is sometimes necessary to manually drop the mutex -/// guard to unlock it as soon as possible. If you need the resource until the end of -/// the scope, this is not needed. +/// It is sometimes necessary to manually drop the mutex guard +/// to unlock it sooner than the end of the enclosing scope. /// /// ``` /// use std::sync::{Arc, Mutex}; From c010e711ca5ec02012afb83c0d99aec9d26a9eea Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sat, 13 Jun 2020 10:11:02 -0700 Subject: [PATCH 7/7] Rewrap comments in Mutex example --- src/libstd/sync/mutex.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index 37c8125b0984a..8478457eabfc2 100644 --- a/src/libstd/sync/mutex.rs +++ b/src/libstd/sync/mutex.rs @@ -108,8 +108,8 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// *guard += 1; /// ``` /// -/// It is sometimes necessary to manually drop the mutex guard -/// to unlock it sooner than the end of the enclosing scope. +/// It is sometimes necessary to manually drop the mutex guard to unlock it +/// sooner than the end of the enclosing scope. /// /// ``` /// use std::sync::{Arc, Mutex}; @@ -139,18 +139,18 @@ use crate::sys_common::poison::{self, LockResult, TryLockError, TryLockResult}; /// // This is the result of some important and long-ish work. /// let result = data.iter().fold(0, |acc, x| acc + x * 2); /// data.push(result); -/// // We drop the `data` explicitly because it's not necessary anymore -/// // and the thread still has work to do. This allow other threads to -/// // start working on the data immediately, without waiting -/// // for the rest of the unrelated work to be done here. +/// // We drop the `data` explicitly because it's not necessary anymore and the +/// // thread still has work to do. This allow other threads to start working on +/// // the data immediately, without waiting for the rest of the unrelated work +/// // to be done here. /// // /// // It's even more important here than in the threads because we `.join` the -/// // threads after that. If we had not dropped the mutex guard, a thread could be -/// // waiting forever for it, causing a deadlock. +/// // threads after that. If we had not dropped the mutex guard, a thread could +/// // be waiting forever for it, causing a deadlock. /// drop(data); -/// // Here the mutex guard is not assigned to a variable and so, even if the scope -/// // does not end after this line, the mutex is still released: -/// // there is no deadlock. +/// // Here the mutex guard is not assigned to a variable and so, even if the +/// // scope does not end after this line, the mutex is still released: there is +/// // no deadlock. /// *res_mutex.lock().unwrap() += result; /// /// threads.into_iter().for_each(|thread| {