Queue async monitor update writes synchronously#4671
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
The code is correct and unchanged from my prior review. The new No issues found. |
tnull
force-pushed
the
2026-06-async-monitor-update-ordering
branch
from
a3967b5 to
8ac52cf
Compare
|
Updated to line-wrap commit messages. |
| }); | ||
| let write_fut: Pin< | ||
| Box<dyn MaybeSendableFuture<Output = Result<(), io::Error>> + 'static>, | ||
| > = Box::pin(self.kv_store.write( |
There was a problem hiding this comment.
I don't actually think this is a bug, the comment is just wrong. Its not crazy to fix this, however, but we should do it like the code in the else block, not by Box::pining.
There was a problem hiding this comment.
Hm, we'd previously create the future in the async move block, which would delay the write by one poll interval of the runtime scheduler, AFAIU?
Now updated to align the approach with the else clause, though still Box::pinning.
There was a problem hiding this comment.
Yes, but that isn't problematic in this case - we're writing the monitor update, there will never be a new monitor update with the same ID. Thinking about this more I believe the previous code was deliberate - it avoids doing the initial poll right away which moves some work out of the main thread.
There was a problem hiding this comment.
avoids doing the initial poll right away which moves some work out of the main thread.
Huh, that seems like a very steep assumption to make. I don't think we have any influence on when the scheduler will poll this again, let alone on which thread it happens?
There was a problem hiding this comment.
Right, I just meant that this will avoid a tiny bit of work on the thread that's doing message processing (in the general case) or the thread hitting send on a payment (in payment cases). That seems like a marginal win, or at least not a loss worth fixing.
|
👋 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. |
Incremental monitor update persistence had its KVStore write call inside the spawned future body, unlike full-monitor writes. Move the write creation into the synchronous setup path so the implementation matches the async KVStore call-order comments. Extend the native async persistence test to assert monitor update writes are visible before the queued persistence futures are polled. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-06-async-monitor-update-ordering
branch
from
8ac52cf to
d0b0dd6
Compare
| &self, monitor_key: &str, update_name: &str, encoded: Vec<u8>, | ||
| ) -> Pin<Box<dyn MaybeSendableFuture<Output = Result<(), io::Error>> + 'static>> { | ||
| let primary = CHANNEL_MONITOR_UPDATE_PERSISTENCE_PRIMARY_NAMESPACE; | ||
| Box::pin(self.kv_store.write(primary, monitor_key, update_name, encoded)) |
There was a problem hiding this comment.
We still ended up with a Box here, can we not avoid it?
There was a problem hiding this comment.
AFAIU, we can once we bump MSRV, but not before.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
I remain not entirely convinced that the change here is better than the existing code. IIRC the way this was written was deliberate - #4671 (comment)
4 participants
Monitor update persistence relies on KVStore write calls being queued in call order. The incremental async update path deferred that write until executor polling, allowing async scheduling to reorder persistence operations relative to later updates or cleanup.
Add a regression test that builds an incremental update future without polling it and verifies that the write has already been queued.
Co-Authored-By: HAL 9000
This finding was discovered by Project Loupe