Drop a pile of Arcs and OutputSweeper::new_with_kv_store_sync
#4119
Conversation
Both `OutputSweeperSync` and `LiquidityManagerSync` added `Arc`s to their internal state to allow returning a reference to that internal state in testing. While this is required to get a forever-lifetime reference to that state, we don't actually need that in testing. Instead, we turn off the borrow checker in the `lightning-background-processor` async tests where required, avoiding the extra heap indirection.
Given the future returned from async BP methods will be concrete on whatever types get used, there's not much reason to require that the arguments be `'static`, we can let Rust figure out if they're `'static` (and thus the returned future is `'static`).
|
👋 Thanks for assigning @joostjager as a reviewer! |
|
Tagging 0.2 as #4111 is a really awk API currently and I'd rather not ship it, but if it slips so be it. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4119 +/- ##
==========================================
+ Coverage 88.52% 88.55% +0.02%
==========================================
Files 179 179
Lines 134270 134310 +40
Branches 134270 134310 +40
==========================================
+ Hits 118865 118939 +74
+ Misses 12655 12612 -43
- Partials 2750 2759 +9
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:
|
There's no need for these given `DefaultTimeProvider` is a unit struct.
TheBlueMatt
force-pushed
the
2025-09-fewer-arcs
branch
from
283cc4e to
d43c7af
Compare
`OutputSweeper::new_with_kv_store_sync` is a pretty strange API - it allows building an async `OutputSweeper` where the only `await`s are on a sync `KVStore`, ie will immediately block until the IO operation completes. While this isn't broken (futures are allowed to take their time, and async runtimes have to handle this, though they often don't handle it particularly well), its pretty weird. It seems to exist largely for `process_events_async_with_kv_store_sync`, which does async `Event` handling but sync `KVStore` operations (like the existing pre-0.2 "async" background processor). Instead, we allow passing an `OutputSweeperSync` to `process_events_async_with_kv_store_sync`, keeping the API consistent such that a user would use the appropriate `OutputSweeper` variant, but fetching the inner async `OutputSweeper` inside the BP.
TheBlueMatt
force-pushed
the
2025-09-fewer-arcs
branch
from
d43c7af to
5e679b4
Compare
| let kv_store = Arc::new(KVStoreSyncWrapper(kv_store_sync)); | ||
|
|
||
| // Yes, you can unsafe { turn off the borrow checker } | ||
| let lm_async: &'static LiquidityManager<_, _, _, _, _, _> = unsafe { |
There was a problem hiding this comment.
This commit has a nice end result, but this - I am not sure how many devs understand it...
There was a problem hiding this comment.
I hope the comment is clear enough? Easy to just update the comment later if it makes it more readable.
|
|
||
| /// A wrapper around a [`KVStoreSync`] that implements the [`KVStore`] trait. It is not necessary to use this type | ||
| /// directly. | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Right, so instead of ref counting, this will now be cloned
| O: Deref, | ||
| K: Deref, | ||
| OS: Deref<Target = OutputSweeper<T, D, F, CF, KVStoreSyncWrapper<K>, L, O>>, | ||
| OS: Deref<Target = OutputSweeperSync<T, D, F, CF, K, L, O>>, |
There was a problem hiding this comment.
Unclear to me now why I didn't do this in the first place.
Mostly a fix for #4111 but also drops a pile of unnecessary Arcs.