From 3acf7e2c9d4896ddfb48e883023cb76f1c4de9a0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 31 Mar 2023 18:08:10 +0000 Subject: [PATCH] Drop the dummy no-std `Condvar` which never sleeps In `no-std`, we exposed `wait` functions which rely on a dummy `Condvar` which never actually sleeps. This is somwhat nonsensical, not to mention confusing to users. Instead, we simply remove the `wait` methods in `no-std` builds. --- lightning/src/ln/channelmanager.rs | 31 +++++++++++----------- lightning/src/sync/debug_sync.rs | 11 -------- lightning/src/sync/nostd_sync.rs | 41 ------------------------------ lightning/src/util/wakers.rs | 23 ++++++++++++++--- 4 files changed, 35 insertions(+), 71 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 7aca81bc0..7babb8595 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7906,6 +7906,7 @@ mod tests { use bitcoin::hashes::Hash; use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; + #[cfg(feature = "std")] use core::time::Duration; use core::sync::atomic::Ordering; use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; @@ -7931,9 +7932,9 @@ mod tests { // All nodes start with a persistable update pending as `create_network` connects each node // with all other nodes to make most tests simpler. - assert!(nodes[0].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(nodes[1].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(nodes[2].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(nodes[0].node.get_persistable_update_future().poll_is_complete()); + assert!(nodes[1].node.get_persistable_update_future().poll_is_complete()); + assert!(nodes[2].node.get_persistable_update_future().poll_is_complete()); let mut chan = create_announced_chan_between_nodes(&nodes, 0, 1); @@ -7947,19 +7948,19 @@ mod tests { &nodes[0].node.get_our_node_id()).pop().unwrap(); // The first two nodes (which opened a channel) should now require fresh persistence - assert!(nodes[0].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(nodes[1].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(nodes[0].node.get_persistable_update_future().poll_is_complete()); + assert!(nodes[1].node.get_persistable_update_future().poll_is_complete()); // ... but the last node should not. - assert!(!nodes[2].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(!nodes[2].node.get_persistable_update_future().poll_is_complete()); // After persisting the first two nodes they should no longer need fresh persistence. - assert!(!nodes[0].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(!nodes[1].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(!nodes[0].node.get_persistable_update_future().poll_is_complete()); + assert!(!nodes[1].node.get_persistable_update_future().poll_is_complete()); // Node 3, unrelated to the only channel, shouldn't care if it receives a channel_update // about the channel. nodes[2].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &chan.0); nodes[2].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &chan.1); - assert!(!nodes[2].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(!nodes[2].node.get_persistable_update_future().poll_is_complete()); // The nodes which are a party to the channel should also ignore messages from unrelated // parties. @@ -7967,8 +7968,8 @@ mod tests { nodes[0].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.1); nodes[1].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.0); nodes[1].node.handle_channel_update(&nodes[2].node.get_our_node_id(), &chan.1); - assert!(!nodes[0].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(!nodes[1].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(!nodes[0].node.get_persistable_update_future().poll_is_complete()); + assert!(!nodes[1].node.get_persistable_update_future().poll_is_complete()); // At this point the channel info given by peers should still be the same. assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info); @@ -7985,8 +7986,8 @@ mod tests { // persisted and that its channel info remains the same. nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &as_update); nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &bs_update); - assert!(!nodes[0].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(!nodes[1].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(!nodes[0].node.get_persistable_update_future().poll_is_complete()); + assert!(!nodes[1].node.get_persistable_update_future().poll_is_complete()); assert_eq!(nodes[0].node.list_channels()[0], node_a_chan_info); assert_eq!(nodes[1].node.list_channels()[0], node_b_chan_info); @@ -7994,8 +7995,8 @@ mod tests { // the channel info has updated. nodes[0].node.handle_channel_update(&nodes[1].node.get_our_node_id(), &bs_update); nodes[1].node.handle_channel_update(&nodes[0].node.get_our_node_id(), &as_update); - assert!(nodes[0].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); - assert!(nodes[1].node.get_persistable_update_future().wait_timeout(Duration::from_millis(1))); + assert!(nodes[0].node.get_persistable_update_future().poll_is_complete()); + assert!(nodes[1].node.get_persistable_update_future().poll_is_complete()); assert_ne!(nodes[0].node.list_channels()[0], node_a_chan_info); assert_ne!(nodes[1].node.list_channels()[0], node_b_chan_info); } diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index edf7c7535..b9f015af6 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -37,11 +37,6 @@ impl Condvar { Condvar { inner: StdCondvar::new() } } - pub fn wait<'a, T>(&'a self, guard: MutexGuard<'a, T>) -> LockResult> { - let mutex: &'a Mutex = guard.mutex; - self.inner.wait(guard.into_inner()).map(|lock| MutexGuard { mutex, lock }).map_err(|_| ()) - } - pub fn wait_while<'a, T, F: FnMut(&mut T) -> bool>(&'a self, guard: MutexGuard<'a, T>, condition: F) -> LockResult> { let mutex: &'a Mutex = guard.mutex; @@ -49,12 +44,6 @@ impl Condvar { .map_err(|_| ()) } - #[allow(unused)] - pub fn wait_timeout<'a, T>(&'a self, guard: MutexGuard<'a, T>, dur: Duration) -> LockResult<(MutexGuard<'a, T>, ())> { - let mutex = guard.mutex; - self.inner.wait_timeout(guard.into_inner(), dur).map(|(lock, _)| (MutexGuard { mutex, lock }, ())).map_err(|_| ()) - } - #[allow(unused)] pub fn wait_timeout_while<'a, T, F: FnMut(&mut T) -> bool>(&'a self, guard: MutexGuard<'a, T>, dur: Duration, condition: F) -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)> { diff --git a/lightning/src/sync/nostd_sync.rs b/lightning/src/sync/nostd_sync.rs index ee3e37502..08d54a939 100644 --- a/lightning/src/sync/nostd_sync.rs +++ b/lightning/src/sync/nostd_sync.rs @@ -1,51 +1,10 @@ pub use ::alloc::sync::Arc; use core::ops::{Deref, DerefMut}; -use core::time::Duration; use core::cell::{RefCell, Ref, RefMut}; use super::{LockTestExt, LockHeldState}; pub type LockResult = Result; -pub struct Condvar {} - -pub struct WaitTimeoutResult(bool); -impl WaitTimeoutResult { - pub fn timed_out(&self) -> bool { self.0 } -} - -impl Condvar { - pub fn new() -> Condvar { - Condvar { } - } - - pub fn wait<'a, T>(&'a self, guard: MutexGuard<'a, T>) -> LockResult> { - Ok(guard) - } - - #[allow(unused)] - pub fn wait_timeout<'a, T>(&'a self, guard: MutexGuard<'a, T>, _dur: Duration) -> LockResult<(MutexGuard<'a, T>, ())> { - Ok((guard, ())) - } - - pub fn wait_while<'a, T, F: FnMut(&mut T) -> bool>(&'a self, mut guard: MutexGuard<'a, T>, mut condition: F) - -> LockResult> { - assert!(!condition(&mut *guard)); - Ok(guard) - } - - #[allow(unused)] - pub fn wait_timeout_while<'a, T, F: FnMut(&mut T) -> bool>(&'a self, mut guard: MutexGuard<'a, T>, dur: Duration, mut condition: F) - -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)> { - if condition(&mut *guard) { - Ok((guard, WaitTimeoutResult(true))) - } else { - Ok((guard, WaitTimeoutResult(false))) - } - } - - pub fn notify_all(&self) {} -} - pub struct Mutex { inner: RefCell } diff --git a/lightning/src/util/wakers.rs b/lightning/src/util/wakers.rs index 0385adc0c..602c2ee04 100644 --- a/lightning/src/util/wakers.rs +++ b/lightning/src/util/wakers.rs @@ -15,11 +15,13 @@ use alloc::sync::Arc; use core::mem; -use crate::sync::{Condvar, Mutex}; +use crate::sync::Mutex; use crate::prelude::*; -#[cfg(any(test, feature = "std"))] +#[cfg(feature = "std")] +use crate::sync::Condvar; +#[cfg(feature = "std")] use std::time::Duration; use core::future::Future as StdFuture; @@ -160,6 +162,7 @@ impl Future { } /// Waits until this [`Future`] completes. + #[cfg(feature = "std")] pub fn wait(self) { Sleeper::from_single_future(self).wait(); } @@ -167,10 +170,19 @@ impl Future { /// Waits until this [`Future`] completes or the given amount of time has elapsed. /// /// Returns true if the [`Future`] completed, false if the time elapsed. - #[cfg(any(test, feature = "std"))] + #[cfg(feature = "std")] pub fn wait_timeout(self, max_wait: Duration) -> bool { Sleeper::from_single_future(self).wait_timeout(max_wait) } + + #[cfg(test)] + pub fn poll_is_complete(&self) -> bool { + let mut state = self.state.lock().unwrap(); + if state.complete { + state.callbacks_made = true; + true + } else { false } + } } use core::task::Waker; @@ -198,10 +210,12 @@ impl<'a> StdFuture for Future { /// A struct which can be used to select across many [`Future`]s at once without relying on a full /// async context. +#[cfg(feature = "std")] pub struct Sleeper { notifiers: Vec>>, } +#[cfg(feature = "std")] impl Sleeper { /// Constructs a new sleeper from one future, allowing blocking on it. pub fn from_single_future(future: Future) -> Self { @@ -252,7 +266,6 @@ impl Sleeper { /// Wait until one of the [`Future`]s registered with this [`Sleeper`] has completed or the /// given amount of time has elapsed. Returns true if a [`Future`] completed, false if the time /// elapsed. - #[cfg(any(test, feature = "std"))] pub fn wait_timeout(&self, max_wait: Duration) -> bool { let (cv, notified_fut_mtx) = self.setup_wait(); let notified_fut = @@ -478,6 +491,7 @@ mod tests { } #[test] + #[cfg(feature = "std")] fn test_dropped_future_doesnt_count() { // Tests that if a Future gets drop'd before it is poll()ed `Ready` it doesn't count as // having been woken, leaving the notify-required flag set. @@ -569,6 +583,7 @@ mod tests { } #[test] + #[cfg(feature = "std")] fn test_multi_future_sleep() { // Tests the `Sleeper` with multiple futures. let notifier_a = Notifier::new(); -- 2.39.5