-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
fix(task): forward signals to spawned sub-processes on unix #27141
Conversation
kill_signal, | ||
), | ||
) | ||
.await |
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.
What's done in this PR is the signal listening is setup at a high level and a KillSignal
from deno_task_shell
is piped through all the code into the task runner. Then when a signal is received it's sent along the KillSignal
(which is kind of like a CancellationToken
, but for sending signals).
runtime/ops/signal.rs
Outdated
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.
Moved to runtime/signal.rs
))] | ||
#[derive(Debug, thiserror::Error)] | ||
#[error("Invalid signal: {0}")] | ||
pub struct InvalidSignalIntError(pub libc::c_int); |
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.
Everything is the same in this file except these new errors and it adds a SIGNAL_NUMS
array for getting a list of signals that the operating system can listen to.
let token = token.child_token(); | ||
let droppable_token = DroppableToken(token.clone()); | ||
let droppable_token = token.clone().drop_guard(); |
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.
Is this change related to "deno 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 think it's ok. It's related to using drop_guards()
everywhere now. I was going to refactor out this code for reuse and then found it was built-in.
async fn listen_ctrl_c(kill_signal: KillSignal) { | ||
while let Ok(()) = tokio::signal::ctrl_c().await { | ||
kill_signal.send(deno_task_shell::SignalKind::SIGINT) | ||
} | ||
} |
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.
IIRC once you install Tokio signal, then it's process wide - ie. once installed it can't be uninstalled - this might lead to problems when lifecycle scripts are executed in deno run
- please check if Ctrl + C works correctly to stop the process, after lifecycle script is run.
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.
Oh yeah, we probably shouldn't do this for lifecycle scripts.
Closes #18445