From 1e6f0d3d5fe38f0dff44c91d339aceb1fd865a71 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 6 Jun 2025 14:37:31 -0700 Subject: [PATCH] Switch PersistenceNotifierGuard persist closure to FnOnce This allows us to move owned and mutable values into the closure, with the assumption that the closure can only be invoked once. As a result, we now have to track `should_persist` as an `Option`, such that we can `take` the owned closure and invoke it within the `PersistenceNotifierGuard::drop` implementation. --- lightning/src/ln/channelmanager.rs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 50ef6d70464..4bd866e7090 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2820,10 +2820,12 @@ enum NotifyOption { /// We allow callers to either always notify by constructing with `notify_on_drop` or choose to /// notify or not based on whether relevant changes have been made, providing a closure to /// `optionally_notify` which returns a `NotifyOption`. -struct PersistenceNotifierGuard<'a, F: FnMut() -> NotifyOption> { +struct PersistenceNotifierGuard<'a, F: FnOnce() -> NotifyOption> { event_persist_notifier: &'a Notifier, needs_persist_flag: &'a AtomicBool, - should_persist: F, + // Always `Some` once initialized, but tracked as an `Option` to obtain the closure by value in + // [`PersistenceNotifierGuard::drop`]. + should_persist: Option, // We hold onto this result so the lock doesn't get released immediately. _read_guard: RwLockReadGuard<'a, ()>, } @@ -2838,20 +2840,20 @@ impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> { /// isn't ideal. fn notify_on_drop( cm: &'a C, - ) -> PersistenceNotifierGuard<'a, impl FnMut() -> NotifyOption> { + ) -> PersistenceNotifierGuard<'a, impl FnOnce() -> NotifyOption> { Self::optionally_notify(cm, || -> NotifyOption { NotifyOption::DoPersist }) } #[rustfmt::skip] - fn optionally_notify NotifyOption, C: AChannelManager>(cm: &'a C, mut persist_check: F) - -> PersistenceNotifierGuard<'a, impl FnMut() -> NotifyOption> { + fn optionally_notify NotifyOption, C: AChannelManager>(cm: &'a C, persist_check: F) + -> PersistenceNotifierGuard<'a, impl FnOnce() -> NotifyOption> { let read_guard = cm.get_cm().total_consistency_lock.read().unwrap(); let force_notify = cm.get_cm().process_background_events(); PersistenceNotifierGuard { event_persist_notifier: &cm.get_cm().event_persist_notifier, needs_persist_flag: &cm.get_cm().needs_persist_flag, - should_persist: move || { + should_persist: Some(move || { // Pick the "most" action between `persist_check` and the background events // processing and return that. let notify = persist_check(); @@ -2862,7 +2864,7 @@ impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> { (_, NotifyOption::SkipPersistHandleEvents) => NotifyOption::SkipPersistHandleEvents, _ => NotifyOption::SkipPersistNoEvents, } - }, + }), _read_guard: read_guard, } } @@ -2878,15 +2880,22 @@ impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> { PersistenceNotifierGuard { event_persist_notifier: &cm.get_cm().event_persist_notifier, needs_persist_flag: &cm.get_cm().needs_persist_flag, - should_persist: persist_check, + should_persist: Some(persist_check), _read_guard: read_guard, } } } -impl<'a, F: FnMut() -> NotifyOption> Drop for PersistenceNotifierGuard<'a, F> { +impl<'a, F: FnOnce() -> NotifyOption> Drop for PersistenceNotifierGuard<'a, F> { fn drop(&mut self) { - match (self.should_persist)() { + let should_persist = match self.should_persist.take() { + Some(should_persist) => should_persist, + None => { + debug_assert!(false); + return; + }, + }; + match should_persist() { NotifyOption::DoPersist => { self.needs_persist_flag.store(true, Ordering::Release); self.event_persist_notifier.notify()