Skip to content

Switch PersistenceNotifierGuard persist closure to FnOnce #3835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2025
Merged

Switch PersistenceNotifierGuard persist closure to FnOnce #3835

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Background question: it seems that in some instances, the persist_check isn't just checking, but also making the actual changes to the chan mgr state. Is that fine, or indicative of the PersistenceNotifierGuard not fully matching its desired use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In almost all cases it is making actual changes to the internal state, that's how we have the context to know whether we should persist or not via the returned NotifyOption

Copy link
Contributor

@joostjager joostjager Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd almost think that the guard itself isn't needed then anymore? I don't have the overview of all uses though.

Would an alternative (not in scope of this PR obviously) be to obtain the guard and then also get some kind of function to set the internal state to needs_persist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That function is exactly what should_persist is doing, but it's less error prone to have each function modify the needs_persist_flag as opposed to having it done in one place by the guard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will take a closer look at current uses of this. Still it feels to me that the general idea of a scope with a guard is to do the manipulation inside that scope, not deeper nested in the should_persist callback.

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<F>,
// We hold onto this result so the lock doesn't get released immediately.
_read_guard: RwLockReadGuard<'a, ()>,
}
Expand All @@ -2838,20 +2840,20 @@ impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> {
/// isn't ideal.
fn notify_on_drop<C: AChannelManager>(
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<F: FnMut() -> NotifyOption, C: AChannelManager>(cm: &'a C, mut persist_check: F)
-> PersistenceNotifierGuard<'a, impl FnMut() -> NotifyOption> {
fn optionally_notify<F: FnOnce() -> 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();
Expand All @@ -2862,7 +2864,7 @@ impl<'a> PersistenceNotifierGuard<'a, fn() -> NotifyOption> {
(_, NotifyOption::SkipPersistHandleEvents) => NotifyOption::SkipPersistHandleEvents,
_ => NotifyOption::SkipPersistNoEvents,
}
},
}),
_read_guard: read_guard,
}
}
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent return in release mode - if this would really happen, do you really want to keep running without any indication in logs etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a logger just for this would be weird. It should be unreachable though, this is also why I preferred just keeping the unwrap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, that was that discussion. I'd also have preferred unreachable.

},
};
match should_persist() {
NotifyOption::DoPersist => {
self.needs_persist_flag.store(true, Ordering::Release);
self.event_persist_notifier.notify()
Expand Down
Loading