rust
rust copied to clipboard
Stabilize `#[unix_sigpipe = "sig_dfl"]` on `fn main()`
I would like to propose that we stabilize #[unix_sigpipe = "sig_dfl"]
.
This PR does that.
By my estimation, merging this PR will close https://github.com/rust-lang/rust/issues/46016.
Tracking issue is https://github.com/rust-lang/rust/issues/97889.
Stabilization report
The #[unix_sigpipe = "sig_dfl"]
attribute has been available in nightly since nightly-2022-09-04
(17 months).
It is being used by rustc and rustdoc themselves, and also by third party code (1, 2).
I have monitored the keyword sigpipe
on rust-lang/rust
during all this time, and no problems have been reported regarding the attribute. To the contrary. It's infrastructure was used to implement a bugfix in a backwards compatible way.
Summary and examples
Everything you need to know about the stabilized attribute should be documented in the reference. PR. Rendered.
Test cases
https://github.com/rust-lang/rust/tree/master/tests/ui/attributes/unix_sigpipe
Edge cases
None that I am aware of.
Note that using #[unix_sigpipe = "sig_dfl"]
and receiving a SIGPIPE
means destructors will not run since the process will be immediately killed. I documented this in The Reference.
Unresolved questions
None. See tracking issue.
Worth noting
This PR does NOT stabilize any other variant of the attribute, namely #[unix_sigpipe = "sig_ign"]
and #[unix_sigpipe = "inherit"]
.
There are many reasons for this:
- Avoids choice paralysis between leaving
SIGPIPE
be the default (SIG_IGN
) and explicitly setting it toSIG_IGN
with#[unix_sigpipe = "sig_ign"]
- I have found no one using
#[unix_sigpipe = "sig_ign"]
nor#[unix_sigpipe = "inherit"]
in the wild, so they are much less tested, and there seems to be no desire for them. - Stabilizing something means committing to its existence for all eternity, so we should not stabilize attributes that are not absolutely needed.
- It leaves the door open to change
fn lang_start()
sigpipe
arg fromu8
tobool
("not set" or "set to sig_dfl"), enabling better lto. Only relevant if we end up deciding not to change the default, and not adding#[unix_sigpipe = "inherit"]
. - These variants have unresolved open questions. See tracking issue.
Potential future work
There is a discussion about changing the default SIGPIPE
handler. See here. Changing the default would likely require us to also stabilize #[unix_sigpipe = "sig_ign"]
. But doing that and discussing changing the default is out of scope of this PR.
Left to do
- [ ] Update the unstable book page of
unix_sigpipe
if we merge this.
FCP
Since we can't close the tracking issue entirely yet, my hope is that we can do an FCP on this PR rather than on the tracking issue.
r? @davidtwco
rustbot has assigned @davidtwco. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
The job x86_64-gnu-llvm-16
failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=Enselic
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_aa70e2ad-6c7c-4b42-94f9-2ffa47696e45
GITHUB_EVENT_NAME=pull_request
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=decfa01d7dae71e6869457bf13c21a5a99d1f413
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_aa70e2ad-6c7c-4b42-94f9-2ffa47696e45
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_aa70e2ad-6c7c-4b42-94f9-2ffa47696e45
GITHUB_TRIGGERING_ACTOR=Enselic
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/120832/merge
GITHUB_WORKFLOW_SHA=decfa01d7dae71e6869457bf13c21a5a99d1f413
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
#12 writing image sha256:8d16271bb6e53ea116d89e7ae4a9985bb120d50e1f9747d759c78724e85182ec done
#12 naming to docker.io/library/rust-ci done
#12 DONE 10.1s
##[endgroup]
Setting extra environment values for docker: --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-16]
##[group]Clock drift check
local time: Fri Feb 9 08:40:16 UTC 2024
network time: Fri, 09 Feb 2024 08:40:16 GMT
network time: Fri, 09 Feb 2024 08:40:16 GMT
##[endgroup]
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure:
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-16', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'build.optimized-compiler-builtins', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-16/bin/llvm-config
configure: llvm.link-shared := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id := 99999999
---
..............................................................................i.i....... 4752/16130
........................................................................................ 4840/16130
.................i..........i............i.............................................. 4928/16130
........................................................................................ 5016/16130
........................................................................F.F............. 5104/16130
........................................................................................ 5280/16130
........................................................................................ 5368/16130
........................................................................................ 5456/16130
.....................................i.................................................. 5544/16130
---
---- [ui] tests/ui/feature-gates/feature-gate-unix_sigpipe-inherit.rs stdout ----
diff of stderr:
1 error[E0658]: the `#[unix_sigpipe = "inherit"]` attribute is an experimental feature
+ --> $DIR/feature-gate-unix_sigpipe-inherit.rs:5:1
3 |
3 |
4 LL | #[unix_sigpipe = "inherit"]
The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-unix_sigpipe-inherit/feature-gate-unix_sigpipe-inherit.stderr
---
status: exit status: 1
command: RUSTC_ICE="0" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/feature-gates/feature-gate-unix_sigpipe-inherit.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-unix_sigpipe-inherit" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-unix_sigpipe-inherit/auxiliary"
stdout: none
--- stderr -------------------------------
error[E0658]: the `#[unix_sigpipe = "inherit"]` attribute is an experimental feature
|
Build completed unsuccessfully in 0:13:09
Build completed unsuccessfully in 0:13:09
LL | #[unix_sigpipe = "inherit"] //~ the `#[unix_sigpipe = "inherit"]` attribute is an experimental feature
|
= note: see issue #97889 <https://github.com/rust-lang/rust/issues/97889> for more information
= help: add `#![feature(unix_sigpipe)]` to the crate attributes to enable
= help: add `#![feature(unix_sigpipe)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0658`.
------------------------------------------
------------------------------------------
---- [ui] tests/ui/feature-gates/feature-gate-unix_sigpipe-sig_ign.rs stdout ----
diff of stderr:
1 error[E0658]: the `#[unix_sigpipe = "sig_ign"]` attribute is an experimental feature
- --> $DIR/feature-gate-unix_sigpipe-sig_ign.rs:3:1
+ --> $DIR/feature-gate-unix_sigpipe-sig_ign.rs:5:1
3 |
4 LL | #[unix_sigpipe = "sig_ign"]
The actual stderr differed from the expected stderr.
The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-unix_sigpipe-sig_ign/feature-gate-unix_sigpipe-sig_ign.stderr
To only update this specific test, also pass `--test-args feature-gates/feature-gate-unix_sigpipe-sig_ign.rs`
error: 1 errors occurred comparing output.
status: exit status: 1
status: exit status: 1
command: RUSTC_ICE="0" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/feature-gates/feature-gate-unix_sigpipe-sig_ign.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-unix_sigpipe-sig_ign" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/feature-gates/feature-gate-unix_sigpipe-sig_ign/auxiliary"
--- stderr -------------------------------
--- stderr -------------------------------
error[E0658]: the `#[unix_sigpipe = "sig_ign"]` attribute is an experimental feature
##[error] --> /checkout/tests/ui/feature-gates/feature-gate-unix_sigpipe-sig_ign.rs:5:1
|
LL | #[unix_sigpipe = "sig_ign"] //~ the `#[unix_sigpipe = "sig_ign"]` attribute is an experimental feature
|
= note: see issue #97889 <https://github.com/rust-lang/rust/issues/97889> for more information
= help: add `#![feature(unix_sigpipe)]` to the crate attributes to enable
= help: add `#![feature(unix_sigpipe)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0658`.
------------------------------------------
I do think we should stabilize "inherit". In particular, if we consider changing the default in a future edition, I'd propose changing the default to "inherit", not to "sig_dfl", so that we don't make the syscall at all.
I've mentioned in the forum that I'm missing an option to ignore both the signal and panic-causing errors in println.
If this attribute wasn't so transparent with low-level unix jargon, maybe it could be used to set the third strategy?
I do think we should stabilize "inherit". In particular, if we consider changing the default in a future edition, I'd propose changing the default to "inherit", not to "sig_dfl", so that we don't make the syscall at all.
I don't think we should stabilize #[unix_sigpipe = "inherit"]
, because if we change the default in a future edition to be to not touch SIGPIPE
at all we would have
fn main() |
SIGPIPE |
---|---|
fn main() |
SIGPIPE not touched |
#[unix_sigpipe = "sig_ign"] fn main() |
SIGPIPE set to SIG_IGN |
#[unix_sigpipe = "sig_dfl"] fn main() |
SIGPIPE set to SIG_DFL |
Maybe we don't even need #[unix_sigpipe = "sig_dfl"]
at all in that case.
So maybe we need to make a decision on https://github.com/rust-lang/rust/issues/62569 after all, before we can stabilize anything. Perhaps summarizing https://github.com/rust-lang/rust/issues/62569 into an RFC is the best way forward here. I think it is, and I hope to get around to it in a not too distant future.
If this attribute wasn't so transparent with low-level unix jargon, maybe it could be used to set the third strategy?
I replied to your post in the forum. In short: I don't think #[unix_sigpipe = "..."]
will be useful for this.
@Enselic What's the harm in stabilizing inherit
even if it subsequently becomes the default?
We allow people to write repr(Rust)
, or pub(self)
, even though those are the defaults.
:umbrella: The latest upstream changes (presumably #120881) made this pull request unmergeable. Please resolve the merge conflicts.
@joshtriplett It is possible I am overestimating the harm of stabilizing an attribute that might not be needed long term. 10 years from now I just think it will be annoying to have to maintain an attribute variant that is not needed. It unnecessarily restricts us.
If we can find a way to turn sigpipe: u8
of fn lang_start()
into a sigpipe: bool
, we will achieve better lto. Meaning that the code for the call to libc::signal
can more easily be removed entirely.
I also think that "no attribute used" is a good way to convey "no special code runs".
If we want to stabilize #[unix_sigpipe = "inherit"]
, I can't help but want to bikeshed the name a bit more. Looking at man 7 signal
we can see that "inherit" happens during fork()
:
A child created via fork(2) inherits a copy of its parent's signal dispositions.
so I don't think "inherit" is a slam-dunk name to use in our case, because the "inherit" mechanism has already been applied when the Rust lang_start()
code runs.
Maybe #[unix_sigpipe = "unchanged"]
is a better name?
Maybe change the default on edition boundary, and only support a Boolean attribute that restores the old behavior for cargo fix
to use?
If we stabilize inherit and skip stabilizing dfl, then people who want dfl can explicitly make the call in main themselves. And then you can remove the dfl option altogether and get your lto win.
If we don’t stabilize inherit, then people who want standard UNIX behavior have no way to get it, other than wait for an unspecified new edition, I guess? Which in reality is another 3 years away, at least?
Discussed in today's @rust-lang/libs meeting: libs does not consider ourselves a blocker for this stabilization.
Rebased to resolve conflicts. Current status:
If we decide not to change the default SIGPIPE
disposition which is my recommendation, there are no blockers to merge this PR after an FCP so that #[unix_sigpipe = "sig_dfl"]
is stabilized.
If we want to change the default SIGPIPE
disposition to "inherit"
over an edition boundary, we probably still want to stabilize #[unix_sigpipe = "sig_dfl"]
, but it is less of a slam dunk because in practice SIG_DFL
will be the default.
We can't stabilize #[unix_sigpipe = "sig_ign"]
yet because currently child processes get SIG_IGN
, but arguably they should get SIG_DFL
since that is what most programs assume, and we explicitly made it that way before. Note that we can implement this without running code right before exec
, see https://github.com/rust-lang/rust/pull/121578 for the trick. This open question is mentioned in the tracking issue now.
We can't stabilize #[unix_sigpipe = "inherit"]
yet because we might want to rename it to e.g. #[unix_sigpipe = "unchanged"]
. This open question is mentioned in the tracking issue now.
On behalf of the Edition 2024 Working Group, even though our soft deadline for accepting editions changes has passed, we're still willing to entertain changing the SIGPIPE default in the 2024 edition, but the hard deadline for the edition cutoff is the end of May, so please make a decision promptly. :)
As someone who writes a lot of software that works by connecting things up with pipes: I strongly encourage at least getting #[unix_sigpipe = "sig_dfl"]
out of nightly.
After reading around in 62569, I understand better now some of the concerns around changing the Rust signal-handling default behavior. While as a UNIX-style command-line tool dev I'd much rather not have been surprised by this difference from other development environments on UNIX, it was not that hard to find out where the surprise was coming from and to resolve it.
Except, that is, for the fact that it's locked me into using nightly for basically all of my Rust development ever. That's really my only beef: it's been years, and I'm still using nightly just for this.
So change the default for edition 2024 or don't... but please, please, at least make it possible for developers of UNIX pipe-based tools to use stable instead of having to take on nightly just to get back to their operating system's default behavior.
As a completely separate concern from what to do about the default behavior, I have some concerns about the attribute syntax itself.
Currently, the attribute describes the syscall that is being performed (setting the signal disposition for SIGPIPE), but this is a very low-level API that may be confusing to people not familiar with the weeds of UNIX pipes. Additionally, #[unix_sigpipe = "sig_ign"]
would be misleading if #121578 lands since we won't actually be using SIG_IGN
at all in that case.
I believe that the attribute should instead describe what it is actually achieving: in this case, changing the behavior of the program when a pipe is broken. Proposed syntax: #[on_broken_pipe = "..."]
or #[unix_on_broken_pipe = "..."]
.
Possible values for the attribute would be:
-
"inherit"
to inherit the behavior from the parent process. -
"kill"
to kill the process with SIGPIPE. -
"error"
to have writes return an error (EPIPE).
:umbrella: The latest upstream changes (presumably #122312) made this pull request unmergeable. Please resolve the merge conflicts.
I'm worried that stabilizing this attribute makes std
even more special than it already is. See my comment on the tracking issue: https://github.com/rust-lang/rust/issues/97889#issuecomment-2007391597
I will look into ways of removing the sigpipe
arg on fn lang_start()
. I will also look into renaming the attribute.
This means stabilization of this attribute will not happen near term, so I will close this PR for now. Thanks all for the feedback 👍