Switch PersistenceNotifierGuard persist closure to FnOnce #3835
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3835 +/- ##
==========================================
- Coverage 89.93% 89.90% -0.03%
==========================================
Files 163 163
Lines 131276 131280 +4
Branches 131276 131280 +4
==========================================
- Hits 118062 118028 -34
- Misses 10529 10556 +27
- Partials 2685 2696 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -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`. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
wpaulino
force-pushed
the
persistence-notifier-fn-once
branch
from
a6a43c2 to
bc420a8
Compare
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
wpaulino
force-pushed
the
persistence-notifier-fn-once
branch
from
bc420a8 to
1bc48ab
Compare
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
force-pushed
the
persistence-notifier-fn-once
branch
from
1bc48ab to
1e6f0d3
Compare
| Some(should_persist) => should_persist, | ||
| None => { | ||
| debug_assert!(false); | ||
| return; |
There was a problem hiding this comment.
Silent return in release mode - if this would really happen, do you really want to keep running without any indication in logs etc?
There was a problem hiding this comment.
Passing a logger just for this would be weird. It should be unreachable though, this is also why I preferred just keeping the unwrap.
There was a problem hiding this comment.
Ah yes, that was that discussion. I'd also have preferred unreachable.
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_persistas anOption, such that we cantakethe owned closure and invoke it within thePersistenceNotifierGuard::dropimplementation.Motivated by #3736 (comment).