Skip to content
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

Issue #182 Fixes a problematic loop in produce_simulation_results #183

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

bjohnson5
Copy link
Collaborator

Resolves Issue #182 by removing set.join_next() from the tokio::select! statement and waiting for tasks to finish outside of the loop. Also does some error handling in the run function in case we encounter an error after starting data collection threads.

@@ -1521,7 +1521,7 @@ mod tests {
/// Alice (100) --- (0) Bob (100) --- (0) Carol (100) --- (0) Dave
///
/// The nodes pubkeys in this chain of channels are provided in-order for easy access.
async fn new(capacity: u64) -> DispatchPaymentTestKit<'a> {
async fn new(capacity: u64) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's squash this commit 👍

Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Thanks for the fix (and detection!!), only one real comment here.

Going to tag a patch once this goes in so that we can get a fresh docker image out.

Comment on lines +569 to +573
while let Some(res) = tasks.join_next().await {
if let Err(e) = res {
log::error!("Task exited with error: {e}.");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's pull this out into a closure to de-dup it a little?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any suggestions on what that closure should look like? It will have to be async which is always tricky to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Async closures are dark magic indeed :') Since we're also using it in produce_simulation_results it can probably just be a utility function instead.

Closure could have been something like this:

let wait_for_tasks = |mut tasks: JoinSet<()>| async move {
        while let Some(res) = tasks.join_next().await {
            if let Err(e) = res {
                log::error!("Task exited with error: {e}.");
            }
        }
};

@@ -1158,7 +1188,7 @@ async fn produce_simulation_results(
tokio::select! {
biased;
_ = listener.clone() => {
return Ok(())
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that you can return a value with break in rust - perhaps we can do this here?

let result = loop .... and then break Ok(()) / break Err(()) and then go on to the joinset wait at the end. Has the advantage of always waiting for everything to close out cleanly vs current approach where we only wait on Ok() exits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is a great idea! Just added that in the latest commit.

Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

tACK, last comment non-blocking (only saves us a few lines).

CPU looking muuch better 😻
Screenshot 2024-04-29 at 8 28 04 AM

@carlaKC carlaKC merged commit 5838d4a into bitcoin-dev-project:main Apr 29, 2024
2 checks passed
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