-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve SIGINT handling in uv run
#11009
Conversation
Frankly, I'm still not entirely sure why we wouldn't forward all signals in the same way that we do with SIGTERM. If it wasn't a huge pain, I would probably do it today? But since it is I might just wait until someone complains that something isn't working? |
A downside of forwarding Ctrl-C on the second send is that a process can receive it three times... but I don't know of programs that have special-casing for that. |
The first SIGINT is not forwarded (but is received because the shell sends it directly). The second SIGINT is forwarded import signal, os;
print(f'PID: {os.getpid()}');
print(f'GPID: {os.getpgid(os.getpid())}');
# We display a newline before SIGINT to separate from ^C in shells
signal.signal(signal.SIGINT, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))
signal.signal(signal.SIGTERM, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))
signal.valid_signals
# Hang until we receive an input, e.g., Ctrl-D
try:
input()
except EOFError:
pass
SIGTERM is forwarded immediately (and only received once)
When the PGID of the child is changed, we forward the first SIGINT import signal, os;
print(f'PID: {os.getpid()}');
# Change PGID
os.setpgid(0, 0)
print(f'GPID: {os.getpgid(os.getpid())}');
signal.signal(signal.SIGINT, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))
signal.signal(signal.SIGTERM, lambda s, _: print(f'{signal.Signals(s).name}', flush=True))
# Sleep for 20s; we can't use `input()` or it breaks output for some reason
try:
import time
time.sleep(20)
except EOFError:
pass
|
46cba66
to
7ea0bdf
Compare
Test coverage pretty painful to add here. |
There should be two functional changes here: - If we receive SIGINT twice, forward it to the child process - If the `uv run` child process changes its PGID, then forward SIGINT Previously, we never forwarded SIGINT to a child process. Instead, we relied on shell to do so.
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.
Nice comment. I buy this. I'm unsure about Windows though.
// signals which has semantic meaning for some programs, i.e., slow exit on the first signal and | ||
// fast exit on the second. The exception to this is if a child process changes its process | ||
// group, in which case the shell will _not_ send SIGINT to the child process and uv must take | ||
// ownership of forwarding the signal. |
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 now I get it. Thanks for explaining this here!
} | ||
|
||
debug!("Received SIGINT, forwarding to child at {child_pid}"); | ||
let _ = signal::kill(child_pid, signal::Signal::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.
Does it make sense to log an error here if one occurs?
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.
Probably yeah. This is the existing behavior so I prefer to retain but we could add logging in a separate patch.
} | ||
}; | ||
} | ||
}?; | ||
|
||
// On Windows, we just ignore the console CTRL_C_EVENT and assume it will always be sent to the | ||
// child by the console. There's not a clear programmatic way to forward the signal anyway. |
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.
Does the signal actually get sent to the child? Or maybe my general question here is, will ^C
still work to terminate the child?
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.
Yeah so this should retain our existing behavior — it's just moved from a platform-wide block to a Windows-specific block. The note is just that we can't implement the more complicated behavior we do above for Unix.
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 also had Aria test this assumption in PowerShell and the initial change adding the Ctrl-C handling fixed various bugs.
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 nice! On the ball.
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.24` -> `0.5.25` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.25`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0525) [Compare Source](astral-sh/uv@0.5.24...0.5.25) ##### Enhancements - Allow installation of manylinux wheels on loongarch64 ([#​10927](astral-sh/uv#10927)) - Allow optional `=` for editables in `requirements.txt` ([#​10954](astral-sh/uv#10954)) - Add Windows aarch64 to the release binaries ([#​10885](astral-sh/uv#10885)) ##### Bug fixes - Use spec-compliant (`128+n`) exit codes for `uv run` and `uv tool run` on Unix ([#​10781](astral-sh/uv#10781)) - Fix best-interpreter lookups when there is an invalid interpreter in the `PATH` ([#​11030](astral-sh/uv#11030)) - Guard against concurrent cache writes on Windows ([#​11007](astral-sh/uv#11007)) - Prioritize package preferences with greater package versions ([#​10963](astral-sh/uv#10963)) - Reject `--editable` flag on non-directory requirements ([#​10994](astral-sh/uv#10994)) - Respect `--no-sources` for `uv pip install` workspace discovery ([#​11003](astral-sh/uv#11003)) - Set `JEMALLOC_SYS_WITH_LG_PAGE=16` in ARM Docker builds ([#​10943](astral-sh/uv#10943)) - Update `riscv64` Python downloads to allow install on `riscv64gc` ([#​10937](astral-sh/uv#10937)) - Fix file persist retries on Windows ([#​11008](astral-sh/uv#11008)) - Fix incorrect error message when specifying `tool.uv.sources.(package).workspace` with other options ([#​11013](astral-sh/uv#11013)) - Improve SIGINT handling in `uv run` ([#​11009](astral-sh/uv#11009)) ##### Documentation - Add `SECURITY` policy ([#​11035](astral-sh/uv#11035)) - Add `Requires-Python` upper bound behavior to the docs ([#​10964](astral-sh/uv#10964)) - Add a troubleshooting section and reproducible example guide ([#​10947](astral-sh/uv#10947)) - Add documentation for `uv add -r` ([#​10926](astral-sh/uv#10926)) - Amend `requires-python` rules in resolver documentation ([#​10993](astral-sh/uv#10993)) - Reference workspaces in `--no-sources` documentation ([#​10995](astral-sh/uv#10995)) - Update documentation for activating virtual environments in different shell ([#​11000](astral-sh/uv#11000)) - Add Docker SHA pinning tip ([#​10955](astral-sh/uv#10955)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xMzcuMiIsInVwZGF0ZWRJblZlciI6IjM5LjEzNy4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
* main: (53 commits) Shorten "Using existing Python versions" nav item so it fits on one line (astral-sh#11077) docs: suggest copy linking for GitLab integration guide (astral-sh#11067) Refactor `uv tool run` hint into separate function (astral-sh#11069) Fix typo in no-deps docs/comments/cli description (astral-sh#11073) Allow `--no-dev --invert` in `uv tree` (astral-sh#11068) Add docs for signal handling (astral-sh#11041) Add a bit more context about SIGTERM and PID 1 (astral-sh#11036) Reflow CLI documentation comments (astral-sh#11040) doc typo: unnecessary backslashes to represent brackets in markdown (astral-sh#11059) Update Dependabot links (astral-sh#11054) Document `gather_credentials` (astral-sh#11024) Link to our MRE documentation in the issue template (astral-sh#11045) Avoid sharing state between universal and non-universal resolves (astral-sh#11051) Mark metadata as dynamic when reading from built wheel cache (astral-sh#11046) Fix formatting of `RUST_LOG` documentation (astral-sh#10053) Bump version to 0.5.25 (astral-sh#11042) Add CVE disclosure to security policy (astral-sh#11037) Guard against concurrent cache writes on Windows (astral-sh#11007) Add SECURITY policy (astral-sh#11035) Improve SIGINT handling in `uv run` (astral-sh#11009) ...
There should be two functional changes here:
uv run
child process changes its PGID, then forward SIGINTPreviously, we never forwarded SIGINT to a child process. Instead, we
relied on shell to do so.
On Windows, we still do nothing but eat the Ctrl-C events we receive.
I cannot see an easy way to send them to the child.
The motivation for these changes should be explained in the comments.
Closes #10952 (in which Ray changes its PGID)
Replaces the (much simpler) #10989 with a more comprehensive approach.
See #6738 (comment) for some previous context.