From 9e44b5b3d73792a90e6d5104601fa89685928271 Mon Sep 17 00:00:00 2001 From: Chirantan Ekbote Date: Fri, 2 Apr 2021 21:35:58 +0900 Subject: [PATCH] sync: Don't sleep while holding a spinlock Drop order in rust is weird. Temporaries created in an if let expression are dropped _after_ the else branch. In this case that meant we were sleeping while holding the spin lock, which could potentially lead to the test hanging ~forever if the thread trying to update the value repeatedly failed to acquire the lock. Move the sleep out of the else branch so that the lock is dropped after checking for the waiter but before the thread goes to sleep. BUG=none TEST=Run unit tests and see that they no longer get randomly stuck for several seconds. Change-Id: I08aa80169071959593bee157acda39411472cf11 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2804870 Tested-by: kokoro Commit-Queue: Chirantan Ekbote Reviewed-by: Dylan Reid Reviewed-by: Daniel Verkamp --- cros_async/src/sync/mu.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cros_async/src/sync/mu.rs b/cros_async/src/sync/mu.rs index bb6ea02636..4f3443fb5f 100644 --- a/cros_async/src/sync/mu.rs +++ b/cros_async/src/sync/mu.rs @@ -1255,12 +1255,14 @@ mod test { fn wake_future(waker: Arc>>) { loop { - if let Some(waker) = waker.lock().take() { - waker.wake(); + if let Some(w) = waker.lock().take() { + w.wake(); return; - } else { - thread::sleep(Duration::from_millis(10)); } + + // This sleep cannot be moved into an else branch because we would end up holding + // the lock while sleeping due to rust's drop ordering rules. + thread::sleep(Duration::from_millis(10)); } }