-
Notifications
You must be signed in to change notification settings - Fork 11
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
CLI flag for disabling interrupts #186
Changes from 6 commits
a6a30db
206b1dc
536277c
c32a72e
a48745c
42bad4c
15c20b3
1507ad3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ pub async fn run_ghci( | |
// This function is pretty tricky! We need to handle shutdowns at each stage, and the process | ||
// is a little different each time, so the `select!`s can't be consolidated. | ||
|
||
let no_interrupt_reloads = opts.no_interrupt_reloads; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This definitely feels awkward. I tried passing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think this is fine. |
||
let mut ghci = Ghci::new(handle.clone(), opts) | ||
.await | ||
.wrap_err("Failed to start `ghci`")?; | ||
|
@@ -113,19 +114,21 @@ pub async fn run_ghci( | |
} | ||
Some(new_event) = receiver.recv() => { | ||
tracing::debug!(?new_event, "Received ghci event from watcher while reloading"); | ||
if should_interrupt(reload_receiver).await { | ||
// Merge the events together so we don't lose progress. | ||
// Then, the next iteration of the loop will pick up the `maybe_event` value | ||
// and respond immediately. | ||
event.merge(new_event); | ||
maybe_event = Some(event); | ||
|
||
// Cancel the in-progress reload. This releases the `ghci` lock to prevent a deadlock. | ||
task.abort(); | ||
|
||
// Send a SIGINT to interrupt the reload. | ||
// NB: This may take a couple seconds to register. | ||
ghci.lock().await.send_sigint().await?; | ||
if !no_interrupt_reloads { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here's our behavior change: if we set this flag, then we do not go into that block |
||
if should_interrupt(reload_receiver).await { | ||
// Merge the events together so we don't lose progress. | ||
// Then, the next iteration of the loop will pick up the `maybe_event` value | ||
// and respond immediately. | ||
event.merge(new_event); | ||
maybe_event = Some(event); | ||
|
||
// Cancel the in-progress reload. This releases the `ghci` lock to prevent a deadlock. | ||
task.abort(); | ||
|
||
// Send a SIGINT to interrupt the reload. | ||
// NB: This may take a couple seconds to register. | ||
ghci.lock().await.send_sigint().await?; | ||
} | ||
} | ||
} | ||
ret = &mut task => { | ||
|
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'm not super happy with a
--no-interrupt-reloads
flag - feels kind of awkward. But--interrupt-reloads
is the default behavior.We could switch the default back to
no-interrupt
, and then this flag can be--interrupt-reloads
.Made a slack convo to ask about this:
https://mercurytechnologies.slack.com/archives/CUJ89R895/p1702483630925389
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.
There's an open issue for automatic negation flags (i.e. adding
--interrupt-reloads
and--no-interrupt-reloads
at once) here:clap-rs/clap#815