Skip to content

Rewrite sync bg processor #3820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Jun 2, 2025

Initial attempt to rewrite the sync bg processor so that it uses the async version of it. Prepares for #3778 where the Persister trait is made async, and wouldn't be callable from the sync bg processor anymore.

Additionally it allows us to get rid of the define_run_body macro.

Various open ends marked by comments on this PR.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 2, 2025

👋 I see @TheBlueMatt was un-assigned.
If you'd like another reviewer assignemnt, please click here.

@@ -340,6 +344,47 @@ impl Sleeper {
}
}

pub struct Sleep {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already Sleeper in the repository, but from_single_future takes a future and isn't just a sleep.

No need to make it complicated using async tasks?
@joostjager joostjager force-pushed the rewrite-sync-bg-processor branch from 27a8ad2 to 74f5af1 Compare June 2, 2025 13:14
}
},
|_| Instant::now(),
|time: &Instant, dur| time.elapsed().as_secs() > dur,
Copy link
Contributor Author

@joostjager joostjager Jun 2, 2025

Choose a reason for hiding this comment

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

Is there a reason why this wasn't done in the first place? All the timer checks seem to be non-blocking.

let waker_clone = waker.clone();

thread::spawn(move || {
thread::sleep(duration);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chatgpt implementation - probably a lot wrong about it.

@joostjager joostjager requested a review from TheBlueMatt June 2, 2025 13:32
@joostjager
Copy link
Contributor Author

@TheBlueMatt is this a viable approach?

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @TheBlueMatt! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@joostjager joostjager removed the request for review from TheBlueMatt June 4, 2025 14:10
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.

2 participants