notify ChainMonitor's EventNotifier on async write completion#4200
notify ChainMonitor's EventNotifier on async write completion#4200tnull merged 1 commit intolightningdevkit:mainfrom
notify ChainMonitor's EventNotifier on async write completion#4200Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
In c084767 we a `ChainMonitor::new_async_beta` and a corresponding `MonitorUpdatingPersisterAsync` struct. To avoid circular references, the `ChainMonitor` owns the `MonitorUpdatingPersisterAsync` which uses a `FutureSpawner` to let async writes happen in the background after it returns control flow ownership to the `ChainMonitor`. This is great, except that because `MonitorUpdatingPersisterAsync` thus doesn't have a reference to the `ChainMonitor`, we have to poll for completion of the futures. We do so in the new `Persist::get_and_clear_completed_updates` that was added in the above-referenced commit. But on async monitor write completion we're supposed to call `ChainMonitor`'s `event_notifier.notify()` (ultimately waking up the background processor which `await`s the corresponding update future). This didn't happen in the new async flow. Here we move `ChainMonitor`'s `event_notifier` into an `Arc` and pass a reference to it through to the `MonitorUpdatingPersisterAsync` which can then directly `notify()` it and wake the background processor.
TheBlueMatt
force-pushed
the
2025-11-async-mon-notify
branch
from
40d891a to
092a50a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4200 +/- ##
=======================================
Coverage 88.85% 88.86%
=======================================
Files 180 180
Lines 137901 137910 +9
Branches 137901 137910 +9
=======================================
+ Hits 122537 122551 +14
+ Misses 12552 12549 -3
+ Partials 2812 2810 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tnull
changed the title
notify ChainMonitor's EventNotifier on async write completion
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Make sense, simple bugfix, landing.
I took the liberty of updating the PR description and title.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
Ha, thanks, sorry. |
|
Backported in #4221 |
3 participants
In c084767 we a
ChainMonitor::new_async_betaand a correspondingMonitorUpdatingPersisterAsyncstruct. To avoid circular references, theChainMonitorowns theMonitorUpdatingPersisterAsyncwhich uses aFutureSpawnerto let async writes happen in the background after it returns control flow ownership to theChainMonitor. This is great, except that becauseMonitorUpdatingPersisterAsyncthus doesn't have a reference to theChainMonitor, we have to poll for completion of the futures.We do so in the new
Persist::get_and_clear_completed_updatesthat was added in the above-referenced commit. But on async monitor write completion we're supposed to callChainMonitor'sevent_notifier.notify()(ultimately waking up the background processor whichawaits the corresponding update future). This didn't happen in the new async flow.Here we move
ChainMonitor'sevent_notifierinto anArcand pass a reference to it through to theMonitorUpdatingPersisterAsyncwhich can then directlynotify()it and wake the background processor.