-
Notifications
You must be signed in to change notification settings - Fork 110
refactor(cli-utils): RuntimeManager API Updates #418
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
Conversation
Consolidate runtime utilities by replacing separate build_runtime(), run_until_ctrl_c(), and run_until_ctrl_c_fallible() methods with a simpler tokio_runtime() and unified run_until_ctrl_c() that handles runtime creation internally. Ported from #395.
✅ Heimdall Review Status
|
haardikk21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question, lgtm otherwise
| res = fut => res, | ||
| _ = tokio::signal::ctrl_c() => { | ||
| tracing::info!(target: "cli", "Received Ctrl-C, shutting down..."); | ||
| Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these get matched on in order right? the prev version was biased towards the ctrl c handler over the future resolving - do we want to maintain that behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good callout, can bias it towards ctrl+c. By default tokio select! fairness is random so it'd make more sense to bias here.
| fn main() -> eyre::Result<()> { | ||
| Version::init(); | ||
| let cli = MyCli::parse(); | ||
| let _cli = MyCli::parse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the commented code sample underscored 😅 is the linter running on these code blocks too somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the linter runs on rust code docs. Can ignore by writing ```rust,ignore
Summary
Ported from #395.
Consolidate runtime utilities by replacing separate
build_runtime(),run_until_ctrl_c(), andrun_until_ctrl_c_fallible()methods with a simplertokio_runtime()and unifiedrun_until_ctrl_c()that handles runtime creation internally.