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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wpaulino
Copy link
Contributor

@wpaulino wpaulino commented Jun 9, 2025

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.

Motivated by #3736 (comment).

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 9, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Copy link

codecov bot commented Jun 9, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.85%. Comparing base (0430b1e) to head (bc420a8).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3835      +/-   ##
==========================================
- Coverage   89.92%   89.85%   -0.08%     
==========================================
  Files         160      161       +1     
  Lines      129581   129868     +287     
  Branches   129581   129868     +287     
==========================================
+ Hits       116525   116689     +164     
- Misses      10361    10481     +120     
- Partials     2695     2698       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tnull tnull self-requested a review June 9, 2025 07:25
@@ -2815,10 +2815,10 @@ 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

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?

@@ -2833,20 +2833,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> {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means that only single-use closure may be provided now. And it seems we are lucky that nowhere a closure was reused, so no other refactoring needed? (questions for me to understand this better)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, and there should never be a closure reuse given the usage pattern

Copy link
Contributor

@joostjager joostjager Jun 11, 2025

Choose a reason for hiding this comment

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

No reuse inside PersistenceNotifierGuard obviously, but I wondered whether there could be a reason to reuse the closure higher up the call stack? (still bg q this, and learning rust)

tnull
tnull previously approved these changes Jun 10, 2025
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, one question

fn drop(&mut self) {
match (self.should_persist)() {
match (self.should_persist.take().unwrap())() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should never reach this, but how about avoiding the unwrap and just matching on Some(..)/None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it basically the same? unreachable would also panic in the None case

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we could also have it trigger a debug_assert, no? But if you prefer the unwrap, could you add a

// unwrap safety: ..

comment?

wpaulino added 2 commits June 10, 2025 10:28
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.
@wpaulino wpaulino force-pushed the persistence-notifier-fn-once branch from a6a43c2 to bc420a8 Compare June 10, 2025 17:28
@wpaulino wpaulino requested review from joostjager and tnull June 10, 2025 17:29
Copy link
Contributor

@optout21 optout21 left a comment

Choose a reason for hiding this comment

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

LGTM. Changing to closure to Option wouldn't have been my obvious solution, but it makes sense.

fn drop(&mut self) {
match (self.should_persist)() {
match (self.should_persist.take().unwrap())() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we could also have it trigger a debug_assert, no? But if you prefer the unwrap, could you add a

// unwrap safety: ..

comment?

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

This needs a rebase now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants