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

added message when exiting #4069

Merged
merged 6 commits into from
Oct 17, 2023
Merged

added message when exiting #4069

merged 6 commits into from
Oct 17, 2023

Conversation

marcellorigotti
Copy link
Contributor

Pull Request

Closes: PRO-xxx

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Added a general message when exiting the program:

  • Exiting chainflip-engine due to error: {ERR}
  • Exiting chainflip-engine due to panic: {PANIC}

@linear
Copy link

linear bot commented Oct 2, 2023

PRO-359 Engine should log an easy to find/understand message when it exits

Currently when the engine exits if it was due to an error we print "Error: {error}" (by returning the result from the main function). This makes the error message hard to find, and unclear when reading the logs (As it is a generic message that doesn't mention the software is exiting). And it doesn't handle the case of panics (Where currently we print nothing at the point the engine exits).

This message should include the error/panic if there was one.

I imagine something like "Exiting chainflip-engine due to …" which would nicely match the current "Starting chainflip-engine" log.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #4069 (1cd5c27) into main (e79b88c) will increase coverage by 0%.
Report is 22 commits behind head on main.
The diff coverage is n/a.

@@          Coverage Diff           @@
##            main   #4069    +/-   ##
======================================
  Coverage     71%     72%            
======================================
  Files        377     378     +1     
  Lines      59967   60663   +696     
  Branches   59967   60663   +696     
======================================
+ Hits       42717   43490   +773     
+ Misses     15008   14907   -101     
- Partials    2242    2266    +24     
Files Coverage Δ
utilities/src/with_std/logging.rs 0% <ø> (ø)

... and 39 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -89,7 +92,25 @@ async fn main() -> anyhow::Result<()> {
// like `--version`), so we execute it only after the settings have been parsed.
utilities::print_starting!();

task_scope(|scope| {
match run_main(settings).await {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest looking at the macro print_starting!, and basically making a macro print_start_and_end that takes an expression something likes

print_start_and_end! {
async {
run_main().await
}
}

You'll need to variants of the macro one for sync code, and one for async code. That way we can do the same thing for the node's usage of print_starting! (i.e. replace it with print_start_and_end).

The only other thing is I suggest putting this around everything, so it is the first and last thing we do in main.

Ok(())
}

async fn run_main(settings: Settings) -> Result<anyhow::Result<()>, Box<dyn Any + Send>> {

std::panic::AssertUnwindSafe(task_scope(|scope| {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move the AssertUnwindSafe, and catch_unwind into the macro. Also with the other usage of the macro.

Copy link
Contributor

@AlastairHolmes AlastairHolmes left a comment

Choose a reason for hiding this comment

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

Great work, thank you.

@marcellorigotti marcellorigotti marked this pull request as ready for review October 17, 2023 09:34
@marcellorigotti marcellorigotti merged commit 4789131 into main Oct 17, 2023
44 checks passed
@marcellorigotti marcellorigotti deleted the feature/pro-359 branch October 17, 2023 11:25
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