Skip to content

Conversation

@tnull
Copy link
Collaborator

@tnull tnull commented Nov 10, 2025

Previously, we'd still use KVStoreSync for persistence of our event queue, which also meant calling the sync persistence through our otherwise-async background processor/event handling flow.

Here we switch our EventQueue persistence to be async, which gets us one step further towards async-everything.

Previously, we'd still use `KVStoreSync` for persistence of our event
queue, which also meant calling the sync persistence through our
otherwise-async background processor/event handling flow.

Here we switch our `EventQueue` persistence to be async, which gets us
one step further towards async-everything.
@tnull tnull requested a review from joostjager November 10, 2025 12:52
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Nov 10, 2025

👋 Thanks for assigning @joostjager 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.

@tnull tnull added this to the 0.7 milestone Nov 10, 2025
@tnull tnull mentioned this pull request Nov 10, 2025
9 tasks
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

Straight forward change


pub(crate) fn add_event(&self, event: Event) -> Result<(), Error> {
{
pub(crate) async fn add_event(&self, event: Event) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at what's left in EventQueue that is sync. Just next_event. Unrelated to this PR, but in every test where that method is used, the test is already async. So it could use the async version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking at what's left in EventQueue that is sync. Just next_event. Unrelated to this PR, but in every test where that method is used, the test is already async. So it could use the async version?

But next_event doesn't do anything that would require async operation? It doesn't repersist or anything?

pub fn event_handled(&self) -> Result<(), Error> {
self.event_queue.event_handled().map_err(|e| {
// We use our runtime for the sync variant to ensure `tokio::task::block_in_place` is
// always called if we'd ever hit this in an outer runtime context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Every time I read this, the same question marks pop up. It feels so strange to have regular sync code and worry about an outer runtime. And then we also make it more complicated because our own block_on variant isn't using the internal handle if there is already a context.

@tnull
Copy link
Collaborator Author

tnull commented Nov 11, 2025

Straight forward change

Not so straightforward after all. This is blocked as the currently the upstream TestStore changes now require to manually complete any writes, i.e., in all these tests we'd need to exactly know what writes there are and which ones to complete

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.

3 participants