From eb98fa9e11138bf4c2f871cdf0af33a9a730ac1f Mon Sep 17 00:00:00 2001
From: Alice Ryhl <aliceryhl@google.com>
Date: Sun, 19 Mar 2023 15:42:19 +0100
Subject: [PATCH 1/3] sync: try to lock the parent first in CancellationToken
Deadlock safety requires that the parent is locked before the child, but
we can still use `try_lock` to attempt to lock the parent without
releasing the child.
---
.../src/sync/cancellation_token/tree_node.rs | 68 +++++++++----------
1 file changed, 32 insertions(+), 36 deletions(-)
diff --git a/tokio-util/src/sync/cancellation_token/tree_node.rs b/tokio-util/src/sync/cancellation_token/tree_node.rs
index 8f97dee8de5..ec55268cc2b 100644
--- a/tokio-util/src/sync/cancellation_token/tree_node.rs
+++ b/tokio-util/src/sync/cancellation_token/tree_node.rs
@@ -151,47 +151,43 @@ fn with_locked_node_and_parent<F, Ret>(node: &Arc<TreeNode>, func: F) -> Ret
where
F: FnOnce(MutexGuard<'_, Inner>, Option<MutexGuard<'_, Inner>>) -> Ret,
{
- let mut potential_parent = {
- let locked_node = node.inner.lock().unwrap();
- match locked_node.parent.clone() {
- Some(parent) => parent,
- // If we locked the node and its parent is `None`, we are in a valid state
- // and can return.
- None => return func(locked_node, None),
- }
- };
+ use std::sync::TryLockError;
+ let mut locked_node = node.inner.lock().unwrap();
+
+ // Every time this fails, the number of ancestors of the node decreases,
+ // so the loop must succeed after a finite number of iterations.
loop {
- // Deadlock safety:
- //
- // Due to invariant #2, we know that we have to lock the parent first, and then the child.
- // This is true even if the potential_parent is no longer the current parent or even its
- // sibling, as the invariant still holds.
- let locked_parent = potential_parent.inner.lock().unwrap();
- let locked_node = node.inner.lock().unwrap();
-
- let actual_parent = match locked_node.parent.clone() {
- Some(parent) => parent,
- // If we locked the node and its parent is `None`, we are in a valid state
- // and can return.
- None => {
- // Was the wrong parent, so unlock it before calling `func`
- drop(locked_parent);
- return func(locked_node, None);
- }
+ // Look up the parent of the currently locked node.
+ let potential_parent = match locked_node.parent.as_ref() {
+ Some(potential_parent) => potential_parent.clone(),
+ None => return func(locked_node, None),
};
- // Loop until we managed to lock both the node and its parent
- if Arc::ptr_eq(&actual_parent, &potential_parent) {
- return func(locked_node, Some(locked_parent));
- }
-
- // Drop locked_parent before reassigning to potential_parent,
- // as potential_parent is borrowed in it
- drop(locked_node);
- drop(locked_parent);
+ // Lock the parent. This may require unlocking the child first.
+ let locked_parent = match potential_parent.inner.try_lock() {
+ Ok(locked_parent) => locked_parent,
+ Err(TryLockError::WouldBlock) => {
+ drop(locked_node);
+ // Deadlock safety:
+ //
+ // Due to invariant #2, the potential parent must come before
+ // the child in the creation order. Therefore, we can safely
+ // lock the child while holding the parent lock.
+ let locked_parent = potential_parent.inner.lock().unwrap();
+ locked_node = node.inner.lock().unwrap();
+ locked_parent
+ },
+ Err(err @ TryLockError::Poisoned(_)) => Err(err).unwrap(),
+ };
- potential_parent = actual_parent;
+ // If we unlocked the child, then the parent may have changed. Check
+ // that we still have the right parent.
+ if let Some(actual_parent) = locked_node.parent.as_ref() {
+ if Arc::ptr_eq(&actual_parent, &potential_parent) {
+ return func(locked_node, Some(locked_parent));
+ }
+ }
}
}
From 088a4826ed2ba4b2100b63e3c819422873be35c8 Mon Sep 17 00:00:00 2001
From: Alice Ryhl <aliceryhl@google.com>
Date: Sun, 19 Mar 2023 15:47:12 +0100
Subject: [PATCH 2/3] rustfmt
---
tokio-util/src/sync/cancellation_token/tree_node.rs | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tokio-util/src/sync/cancellation_token/tree_node.rs b/tokio-util/src/sync/cancellation_token/tree_node.rs
index ec55268cc2b..b75f4b90058 100644
--- a/tokio-util/src/sync/cancellation_token/tree_node.rs
+++ b/tokio-util/src/sync/cancellation_token/tree_node.rs
@@ -177,14 +177,14 @@ where
let locked_parent = potential_parent.inner.lock().unwrap();
locked_node = node.inner.lock().unwrap();
locked_parent
- },
+ }
Err(err @ TryLockError::Poisoned(_)) => Err(err).unwrap(),
};
// If we unlocked the child, then the parent may have changed. Check
// that we still have the right parent.
if let Some(actual_parent) = locked_node.parent.as_ref() {
- if Arc::ptr_eq(&actual_parent, &potential_parent) {
+ if Arc::ptr_eq(actual_parent, &potential_parent) {
return func(locked_node, Some(locked_parent));
}
}
From 3735aaac85628525a423a510cf55a66c1c5269e6 Mon Sep 17 00:00:00 2001
From: Alice Ryhl <aliceryhl@google.com>
Date: Sun, 19 Mar 2023 15:48:35 +0100
Subject: [PATCH 3/3] use PoisonError directly
---
tokio-util/src/sync/cancellation_token/tree_node.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tokio-util/src/sync/cancellation_token/tree_node.rs b/tokio-util/src/sync/cancellation_token/tree_node.rs
index b75f4b90058..f1abb4a8d33 100644
--- a/tokio-util/src/sync/cancellation_token/tree_node.rs
+++ b/tokio-util/src/sync/cancellation_token/tree_node.rs
@@ -178,7 +178,7 @@ where
locked_node = node.inner.lock().unwrap();
locked_parent
}
- Err(err @ TryLockError::Poisoned(_)) => Err(err).unwrap(),
+ Err(TryLockError::Poisoned(err)) => Err(err).unwrap(),
};
// If we unlocked the child, then the parent may have changed. Check