Skip to content
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

Tracking Issue for unix_sigpipe #97889

Open
11 of 23 tasks
Enselic opened this issue Jun 8, 2022 · 23 comments
Open
11 of 23 tasks

Tracking Issue for unix_sigpipe #97889

Enselic opened this issue Jun 8, 2022 · 23 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-needs-deep-research Status: This feature needs deep research to solve design or implementation issues. S-tracking-needs-documentation Status: Needs documentation. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Enselic
Copy link
Member

Enselic commented Jun 8, 2022




↓↓↓↓ Important ↓↓↓↓

The ui changed from an attribute to a compiler flag: #124480, so the below description is out of date. Someone (maybe me) should update the description.

↑↑↑↑ Important ↑↑↑↑




The feature gate for the issue is #![feature(unix_sigpipe)].
It enables a new fn main() attribute #[unix_sigpipe = "..."].

Usage

Any simple Rust program that writes a sizeable amount of data to stdout will panic if its output is limited via pipes.

fn main() {
    loop {
        println!("hello world");
    }
}
% ./main | head -n 1
hello world
thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrac

To prevent panicking we can use the new attribute:

#![feature(unix_sigpipe)]

#[unix_sigpipe = "sig_dfl"]
fn main() {
    loop {
        println!("hello world");
    }
}
% ./main | head -n 1
hello world

More Info

Please refer to the unstable book section for more details. In short:

#[unix_sigpipe = "..."] Behaviour
sig_ign Set SIGPIPE handler to SIG_IGN before invoking fn main(). Default behaviour since 2014.
sig_dfl Set SIGPIPE handler to SIG_DFL before invoking fn main().
inherit Leave SIGPIPE handler untounched before entering fn main().

The problem with the current SIGPIPE code in libstd as well as several other aspects of this problem is discussed extensively at these places:

Naming convention

The naming follows the convention used by #![windows_subsystem = "windows|console"] where the values "windows" and "console" have the same names as the actual linker flags: /SUBSYSTEM:WINDOWS and /SUBSYSTEM:CONSOLE.

The names sig_ign and sig_dfl comes from the signal handler names SIG_IGN and SIG_DFL.

Steps

Unresolved Questions That Blocks Stabilisation

  • How can we make it easy to put fn lang_start() in an external crate that can be compiled with stable?
  • We should rename the attribute and attribute values to things that reflect what they do rather than how they do it.
  • #[unix_sigpipe = "sig_dfl"]
    • None
  • #[unix_sigpipe = "sig_ign"]
  • #[unix_sigpipe = "inherit"]
    • Is the name clear enough? Maybe rename to unchanged?

Unresolved Questions That Does Not Block Stabilisation

Because these questions can be resolved incrementally after stabilization.

Resolved Questions

  • We don't want to change to a -Z unix_sigpipe flag instead, see https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Proposal.3A.20First.20step.20towards.20solving.20the.20SIGPIPE.20problem/near/285499895, at least not initially.
  • Should we only have the third sigpipe: u8 argument to fn lang_start() on Unix platform via cfg?
    Answer: No, this is not allowed, see top level comment in https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/pal.rs
  • Should we stabilize sig_dfl or is inherit and sig_ign sufficient?
    Answer: There are noteworthy examples of real projects that has opted to use SIG_DFL to solve the BrokenPipe problem. Notably rustc itself. So if we don't stabilize sig_dfl, such projects can't make use of our new attribute. Therefore, we also need to stabilize sig_dfl.
  • Should the attribute go on fn main() or on the top-level module (#![unix_sigpipe="..."])?
    Answer: It makes a lot of semantic sense to have the attribute on fn main(), because it is a way to configure what the Rust runtime should do before fn main() is invoked. For libraries, no entry point code that modifies SIGPIPE is generated, so allowing the attribute in these situations does not make much sense. See Change process spawning to inherit the parent's signal mask by default #101077 (comment) for small-scale discussion.
  • Can we write the code in a way that allows lto to remove the _signal stub code completely? With a bool it works (see Support #[unix_sigpipe = "inherit|sig_dfl"] on fn main() to prevent ignoring SIGPIPE #97802 (comment)), but with the current u8 we might need to do some tweaks. Answer: There are currently 4 values and I see no feasible way to reduce it to 2.
  • Can and should we alter the BrokenPipe error message and make it suggest to use the new attribute? Answer: No, because that would mean we would end up giving developer advice to users that can't act on the advice.
  • Does this have any impact on defining a stable ABI? Answer: No, because ABI discussion are about enabling things such as calling Rust functions in binary artifacts produced by an older Rust compiler than the current one. That we changed the ABI of fn lang_start() is not relevant. And a stable Rust ABI is not even close (see Define a Rust ABI rfcs#600).
  • Can we use MSG_NOSIGNAL with send() etc instead of setting SIGPIPE globally? Answer: No, because there is no equivalent for write(), and it would incur an extra syscall for each write-operation, which is likely to have significant performance drawbacks.

Disclaimer: I have taken the liberty to mark some questions resolved that I find unlikely to be controversial. If you would like me to create a proper discussion ticket for any of the resolved or unresolved questions, please let me know!

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

@rustbot label +T-libs

@Enselic Enselic added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jun 8, 2022
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 8, 2022
@Enselic Enselic changed the title Tracking Issue for no_ignore_sigpipe Tracking Issue for #![sigpipe_handler(sig_ign|unchanged)] Jun 10, 2022
@Enselic Enselic changed the title Tracking Issue for #![sigpipe_handler(sig_ign|unchanged)] Tracking Issue for pipeable Jun 13, 2022
@Enselic Enselic changed the title Tracking Issue for pipeable Tracking Issue for sigpipe Jun 16, 2022
@Enselic Enselic changed the title Tracking Issue for sigpipe Tracking Issue for unix_sigpipe Jun 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 2, 2022
…, r=joshtriplett

Support `#[unix_sigpipe = "inherit|sig_dfl"]` on `fn main()` to prevent ignoring `SIGPIPE`

When enabled, programs don't have to explicitly handle `ErrorKind::BrokenPipe` any longer. Currently, the program

```rust
fn main() { loop { println!("hello world"); } }
```

will print an error if used with a short-lived pipe, e.g.

    % ./main | head -n 1
    hello world
    thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

by enabling `#[unix_sigpipe = "sig_dfl"]` like this

```rust
#![feature(unix_sigpipe)]
#[unix_sigpipe = "sig_dfl"]
fn main() { loop { println!("hello world"); } }
```

there is no error, because `SIGPIPE` will not be ignored and thus the program will be killed appropriately:

    % ./main | head -n 1
    hello world

The current libstd behaviour of ignoring `SIGPIPE` before `fn main()` can be explicitly requested by using `#[unix_sigpipe = "sig_ign"]`.

With `#[unix_sigpipe = "inherit"]`, no change at all is made to `SIGPIPE`, which typically means the behaviour will be the same as `#[unix_sigpipe = "sig_dfl"]`.

See rust-lang#62569 and referenced issues for discussions regarding the `SIGPIPE` problem itself

See the [this](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Proposal.3A.20First.20step.20towards.20solving.20the.20SIGPIPE.20problem) Zulip topic for more discussions, including about this PR.

Tracking issue: rust-lang#97889
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Sep 4, 2022
…riplett

Support `#[unix_sigpipe = "inherit|sig_dfl"]` on `fn main()` to prevent ignoring `SIGPIPE`

When enabled, programs don't have to explicitly handle `ErrorKind::BrokenPipe` any longer. Currently, the program

```rust
fn main() { loop { println!("hello world"); } }
```

will print an error if used with a short-lived pipe, e.g.

    % ./main | head -n 1
    hello world
    thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

by enabling `#[unix_sigpipe = "sig_dfl"]` like this

```rust
#![feature(unix_sigpipe)]
#[unix_sigpipe = "sig_dfl"]
fn main() { loop { println!("hello world"); } }
```

there is no error, because `SIGPIPE` will not be ignored and thus the program will be killed appropriately:

    % ./main | head -n 1
    hello world

The current libstd behaviour of ignoring `SIGPIPE` before `fn main()` can be explicitly requested by using `#[unix_sigpipe = "sig_ign"]`.

With `#[unix_sigpipe = "inherit"]`, no change at all is made to `SIGPIPE`, which typically means the behaviour will be the same as `#[unix_sigpipe = "sig_dfl"]`.

See rust-lang/rust#62569 and referenced issues for discussions regarding the `SIGPIPE` problem itself

See the [this](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Proposal.3A.20First.20step.20towards.20solving.20the.20SIGPIPE.20problem) Zulip topic for more discussions, including about this PR.

Tracking issue: rust-lang/rust#97889
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…riplett

Support `#[unix_sigpipe = "inherit|sig_dfl"]` on `fn main()` to prevent ignoring `SIGPIPE`

When enabled, programs don't have to explicitly handle `ErrorKind::BrokenPipe` any longer. Currently, the program

```rust
fn main() { loop { println!("hello world"); } }
```

will print an error if used with a short-lived pipe, e.g.

    % ./main | head -n 1
    hello world
    thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

by enabling `#[unix_sigpipe = "sig_dfl"]` like this

```rust
#![feature(unix_sigpipe)]
#[unix_sigpipe = "sig_dfl"]
fn main() { loop { println!("hello world"); } }
```

there is no error, because `SIGPIPE` will not be ignored and thus the program will be killed appropriately:

    % ./main | head -n 1
    hello world

The current libstd behaviour of ignoring `SIGPIPE` before `fn main()` can be explicitly requested by using `#[unix_sigpipe = "sig_ign"]`.

With `#[unix_sigpipe = "inherit"]`, no change at all is made to `SIGPIPE`, which typically means the behaviour will be the same as `#[unix_sigpipe = "sig_dfl"]`.

See rust-lang/rust#62569 and referenced issues for discussions regarding the `SIGPIPE` problem itself

See the [this](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Proposal.3A.20First.20step.20towards.20solving.20the.20SIGPIPE.20problem) Zulip topic for more discussions, including about this PR.

Tracking issue: rust-lang/rust#97889
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 24, 2022
…h726

rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`

This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead.

For now we just start using `unix_sigpipe` in one place: `rustc` itself.

It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature.

`@rustdoc` labels +T-libs
JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Oct 24, 2022
…triddle

rustdoc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`

Do what was already done for `rustc` in rust-lang#102587, namely start using `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`.

After this has been merged, we can completely remove `rustc_driver::set_sigpipe_handler`.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Verification of this change
---------------------------

1. Remove `#[unix_sigpipe = "sig_dfl"]`
1. Run `./x.py build`
1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false`
1. Observe ICE
1. Add back `#[unix_sigpipe = "sig_dfl"]`
1. Run `./x.py build`
1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false`
1. Observe ICE fixed

`@rustbot` labels +T-rustdoc
JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Oct 24, 2022
…triddle

rustdoc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`

Do what was already done for `rustc` in rust-lang#102587, namely start using `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`.

After this has been merged, we can completely remove `rustc_driver::set_sigpipe_handler`.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Verification of this change
---------------------------

1. Remove `#[unix_sigpipe = "sig_dfl"]`
1. Run `./x.py build`
1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false`
1. Observe ICE
1. Add back `#[unix_sigpipe = "sig_dfl"]`
1. Run `./x.py build`
1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false`
1. Observe ICE fixed

``@rustbot`` labels +T-rustdoc
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
…h726

rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`

This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead.

For now we just start using `unix_sigpipe` in one place: `rustc` itself.

It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature.

``@rustdoc`` labels +T-libs
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
…h726

rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`

This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead.

For now we just start using `unix_sigpipe` in one place: `rustc` itself.

It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature.

```@rustdoc``` labels +T-libs
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 25, 2022
…h726

rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`

This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead.

For now we just start using `unix_sigpipe` in one place: `rustc` itself.

It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature.

````@rustdoc```` labels +T-libs
compiler-errors added a commit to compiler-errors/rust that referenced this issue Oct 25, 2022
…, r=tmiasko

Remove `rustc_driver::set_sigpipe_handler()`

Its usage was removed in rust-lang#102587 and rust-lang#103495, so we do not need to keep it around any longer. According to [preliminary input](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Find.20.60rustc_driver.60.20dependent.20projects.3F/near/304490764), we do not need to worry about any deprecation cycle for this explicitly unstable API, and can just straight up remove it.

PR that added `set_sigpipe_handler`: rust-lang#49606

Tracking issue for `unix_sigpipe`: rust-lang#97889

Migration instructions for any remaining clients
---

Change from

```rust
#![feature(rustc_private)]

extern crate rustc_driver;

fn main() {
    rustc_driver::set_sigpipe_handler();
    // ...
```

to

```rust
#![feature(unix_sigpipe)]

#[unix_sigpipe = "sig_dfl"]
fn main() {
    // ...
```

`@rustbot` labels +T-compiler
wcampbell0x2a added a commit to wcampbell0x2a/scuba that referenced this issue May 8, 2024
Rust ignores SIGPIPE by default, this patch overrides that behaviour to fix
this by *not* panic'ing on Broken pipes and restore old scubainit behavior.

In short, the following fails:
> image: debian:latest
>
> aliases:
>  test: yes '' | echo "test"

$ scuba test:
> test
> yes: standard output: Broken pipe

* Use nightly on-broken-pipe="inherit" to inherit the behavior from the parent process,
  Instead of killing our process. See docs here:
  https://github.com/rust-lang/rust/blob/master/src/doc/unstable-book/src/compiler-flags/on-broken-pipe.md
  This nightly fix is definitely unstable(with very recent API changes),
  but hopefully they keep this interface as a compiler option the same
  until stabilization.

* Add test to verify this doesn't break in the future

* Add rust-toolchain.toml to define the locked rust version since this requires a
  nightly option. I specifically didn't think nightly was an issue, since you
  were looking into using -Zbuild-std for scubainit size minimization
  (which has a long path to stabilization)

This has been a longstanding issue for the Rust language:
- rust-lang/rust#62569
- rust-lang/rust#97889
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue May 18, 2024
Change `SIGPIPE` ui from `#[unix_sigpipe = "..."]` to `-Zon-broken-pipe=...`

In the stabilization [attempt](rust-lang/rust#120832) of `#[unix_sigpipe = "sig_dfl"]`, a concern was [raised ](rust-lang/rust#120832 (comment)) related to using a language attribute for the feature: Long term, we want `fn lang_start()` to be definable by any crate, not just libstd. Having a special language attribute in that case becomes awkward.

So as a first step towards the next stabilization attempt, this PR changes the `#[unix_sigpipe = "..."]` attribute to a compiler flag `-Zon-broken-pipe=...` to remove that concern, since now the language is not "contaminated" by this feature.

Another point was [also raised](rust-lang/rust#120832 (comment)), namely that the ui should not leak **how** it does things, but rather what the **end effect** is. The new flag uses the proposed naming. This is of course something that can be iterated on further before stabilization.

Tracking issue: rust-lang/rust#97889
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 20, 2024
Rust pre-main code may change the SIGPIPE disposition to ignore:
* rust-lang/rust#62569
* rust-lang/rust#97889

We could use the nightly compiler flag -Zon-broken-pipe=inherit to
disable this behavior. Instead, we take the simpler route and restore
the default disposition ourselves.

Fixes #254
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 20, 2024
Rust pre-main code may change the SIGPIPE disposition to ignore:
* rust-lang/rust#62569
* rust-lang/rust#97889

We could use the nightly compiler flag -Zon-broken-pipe=inherit to
disable this behavior. Instead, we take the simpler route and restore
the default disposition ourselves.

Fixes #254
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 22, 2024
Rust pre-main code may change the SIGPIPE disposition to ignore:
* rust-lang/rust#62569
* rust-lang/rust#97889

We could use the nightly compiler flag -Zon-broken-pipe=inherit to
disable this behavior. Instead, we take the simpler route and restore
the default disposition ourselves.

Fixes #254
@jpmckinney
Copy link

For anyone else seeing this on nightly:

error: cannot find attribute `unix_sigpipe` in this scope
   --> src/main.rs:137:3
    |
137 | #[unix_sigpipe = "sig_dfl"]
    |   ^^^^^^^^^^^^

error[E0635]: unknown feature `unix_sigpipe`
 --> src/main.rs:1:12
  |
1 | #![feature(unix_sigpipe)]
  |            ^^^^^^^^^^^^

It seems like this feature is gone and is replaced with a compiler flag: #124480

Note that when doing a web search for these language features or compiler flags, it's not obvious which version of the Unstable Book you're looking at.

Nightly: https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/on-broken-pipe.html
Beta: https://doc.rust-lang.org/beta/unstable-book/language-features/unix-sigpipe.html

JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 29, 2024
Rust pre-main code may change the SIGPIPE disposition to ignore:
* rust-lang/rust#62569
* rust-lang/rust#97889

We could use the nightly compiler flag -Zon-broken-pipe=inherit to
disable this behavior. Instead, we take the simpler route and restore
the default disposition ourselves.

Fixes #254
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 2, 2024
…r-ozkan

Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags"

In [rust-lang#131059] we found out that `-Zon-broken-pipe=kill` is actually **load-bearing**[^1] for
(at least) `rustc` and `rustdoc` to have the kill-process-on-broken-pipe behavior, e.g. `rustc
--print=sysroot | false` will ICE and `rustdoc --print=sysroot | false` will panic on a broken pipe.

This PR reverts 5a7058c (reverts PR rust-lang#131060) in favor of a future
fix to *unconditionally* apply `-Zon-broken-pipe=kill` to tool builds and also not drop the
`-Zon-broken-pipe=kill` flag for rustc binary builds.

I could not figure out how to write a regression test for the `rustc --print=sysroot | false`
behavior on Unix, so this is a plain revert for now.

This revert will unfortunately reintroduce rust-lang#130980 until we fix it again with the different approach.

See more details at <rust-lang#131059 (comment)> and in the timeline below.

### Timeline of kill-process-on-broken-pipe behavior changes

See [`unix_sigpipe` tracking issue rust-lang#97889][rust-lang#97889] for more context around unix sigpipe handling.

- From the very beginning since 2014, Rust binaries by default use `sig_ign`. This meant that if
  output pipe is broken yet the program tries to use `println!` and such, there will be a broken
  pipe panic from std. This lead to ICEs from e.g. `rustc --help | false` [rust-lang#34376].
- [rust-lang#49606] mitigated [rust-lang#34376] by adding an explicit signal handler to `rustc_driver` register a
  sigpipe handler with `SIG_DFL` which will cause the binary using `rustc_driver` to terminate if
  `rustc_driver::set_sigpipe_handler()` is called. `rustc`'s main binary wrapper uses
  `rustc_driver::set_sigpipe_handler()`, and so does `rustdoc`.
- A more universal way to set sigpipe behavior for Unix was introduced as part of [rust-lang#97889], i.e. `#
  [unix_sigpipe = "sig_dfl"]` attribute.
- [rust-lang#102587] migrated `rustc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`.
- [rust-lang#103495] migrated `rustdoc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`. `rustc_driver::set_sigpipe_handler` was removed.
- Following concerns about sigpipe setting UI in [rust-lang#97889], the UI for specifying sigpipe behavior
  was changed in [rust-lang#124480] from `#[unix_sigpipe = "sig_dfl"]` attribute to the commmand line flag
  `-Zon-broken-pipe=kill`.
    - In the same PR, `#[unix_sigpipe = "sig_dfl"]` were removed from `rustc` and `rustdoc` main
      binary crate entry points in favor of the command line flag. Kill-process-on-broken-pipe
      behavior was preserved by adding `-Zon-broken-pipe=kill` for `rustdoc` tool build step and
      `rustc` during compile steps.
- [rust-lang#126934] added `-Zon-broken-pipe=kill` for tool builds *except* for cargo to help with some miri
  tests because at the time the PR was written, this would lead to a couple of cargo test failures.
  Conditionally setting `RUSTFLAGS` can lead to tool build invalidation, e.g. building `cargo`
  without `-Zon-broken-pipe=kill` but `clippy` with the flag can lead to invalidation of the tool
  build cache. This is not a problem at the time, because nothing (not even miri) tests built stage
  1 cargo (all used initial cargo).
- In [rust-lang#130634] we found out that `run-make` tests like `compiler-builtins` needed stage 1 cargo, not
  just beta bootstrap cargo, because there can be changes that are present in stage 1 cargo but
  absent in beta cargo, which was blocking a beta backport.
- [rust-lang#130642] and later [rust-lang#130739] now build stage 1 cargo. And as previously mentioned, since
  `-Zon-broken-pipe=kill` was specifically *not* set for cargo, this caused tool build cache
  invalidation meaning rebuilds of stage 1 even if nothing in source was changed due to differing
  `RUSTFLAGS` since `run-make` also builds `rustdoc` and such [rust-lang#130980].

[rust-lang#34376]: rust-lang#34376
[rust-lang#49606]: rust-lang#49606
[rust-lang#97889]: rust-lang#97889
[rust-lang#102587]: rust-lang#102587
[rust-lang#103495]: rust-lang#103495
[rust-lang#124480]: rust-lang#124480
[rust-lang#130634]: rust-lang#130634
[rust-lang#130642]: rust-lang#130642
[rust-lang#130739]: rust-lang#130739
[rust-lang#130980]: rust-lang#130980
[rust-lang#131059]: rust-lang#131059
[^1]: rust-lang#131059 (comment)

r? `@onur-ozkan` (or bootstrap)
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Oct 2, 2024
…r-ozkan

Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags"

In [rust-lang#131059] we found out that `-Zon-broken-pipe=kill` is actually **load-bearing**[^1] for
(at least) `rustc` and `rustdoc` to have the kill-process-on-broken-pipe behavior, e.g. `rustc
--print=sysroot | false` will ICE and `rustdoc --print=sysroot | false` will panic on a broken pipe.

This PR reverts 5a7058c (reverts PR rust-lang#131060) in favor of a future
fix to *unconditionally* apply `-Zon-broken-pipe=kill` to tool builds and also not drop the
`-Zon-broken-pipe=kill` flag for rustc binary builds.

I could not figure out how to write a regression test for the `rustc --print=sysroot | false`
behavior on Unix, so this is a plain revert for now.

This revert will unfortunately reintroduce rust-lang#130980 until we fix it again with the different approach.

See more details at <rust-lang#131059 (comment)> and in the timeline below.

### Timeline of kill-process-on-broken-pipe behavior changes

See [`unix_sigpipe` tracking issue rust-lang#97889][rust-lang#97889] for more context around unix sigpipe handling.

- From the very beginning since 2014, Rust binaries by default use `sig_ign`. This meant that if
  output pipe is broken yet the program tries to use `println!` and such, there will be a broken
  pipe panic from std. This lead to ICEs from e.g. `rustc --help | false` [rust-lang#34376].
- [rust-lang#49606] mitigated [rust-lang#34376] by adding an explicit signal handler to `rustc_driver` register a
  sigpipe handler with `SIG_DFL` which will cause the binary using `rustc_driver` to terminate if
  `rustc_driver::set_sigpipe_handler()` is called. `rustc`'s main binary wrapper uses
  `rustc_driver::set_sigpipe_handler()`, and so does `rustdoc`.
- A more universal way to set sigpipe behavior for Unix was introduced as part of [rust-lang#97889], i.e. `#
  [unix_sigpipe = "sig_dfl"]` attribute.
- [rust-lang#102587] migrated `rustc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`.
- [rust-lang#103495] migrated `rustdoc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`. `rustc_driver::set_sigpipe_handler` was removed.
- Following concerns about sigpipe setting UI in [rust-lang#97889], the UI for specifying sigpipe behavior
  was changed in [rust-lang#124480] from `#[unix_sigpipe = "sig_dfl"]` attribute to the commmand line flag
  `-Zon-broken-pipe=kill`.
    - In the same PR, `#[unix_sigpipe = "sig_dfl"]` were removed from `rustc` and `rustdoc` main
      binary crate entry points in favor of the command line flag. Kill-process-on-broken-pipe
      behavior was preserved by adding `-Zon-broken-pipe=kill` for `rustdoc` tool build step and
      `rustc` during compile steps.
- [rust-lang#126934] added `-Zon-broken-pipe=kill` for tool builds *except* for cargo to help with some miri
  tests because at the time the PR was written, this would lead to a couple of cargo test failures.
  Conditionally setting `RUSTFLAGS` can lead to tool build invalidation, e.g. building `cargo`
  without `-Zon-broken-pipe=kill` but `clippy` with the flag can lead to invalidation of the tool
  build cache. This is not a problem at the time, because nothing (not even miri) tests built stage
  1 cargo (all used initial cargo).
- In [rust-lang#130634] we found out that `run-make` tests like `compiler-builtins` needed stage 1 cargo, not
  just beta bootstrap cargo, because there can be changes that are present in stage 1 cargo but
  absent in beta cargo, which was blocking a beta backport.
- [rust-lang#130642] and later [rust-lang#130739] now build stage 1 cargo. And as previously mentioned, since
  `-Zon-broken-pipe=kill` was specifically *not* set for cargo, this caused tool build cache
  invalidation meaning rebuilds of stage 1 even if nothing in source was changed due to differing
  `RUSTFLAGS` since `run-make` also builds `rustdoc` and such [rust-lang#130980].

[rust-lang#34376]: rust-lang#34376
[rust-lang#49606]: rust-lang#49606
[rust-lang#97889]: rust-lang#97889
[rust-lang#102587]: rust-lang#102587
[rust-lang#103495]: rust-lang#103495
[rust-lang#124480]: rust-lang#124480
[rust-lang#130634]: rust-lang#130634
[rust-lang#130642]: rust-lang#130642
[rust-lang#130739]: rust-lang#130739
[rust-lang#130980]: rust-lang#130980
[rust-lang#131059]: rust-lang#131059
[^1]: rust-lang#131059 (comment)

r? ``@onur-ozkan`` (or bootstrap)
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 2, 2024
Rollup merge of rust-lang#131108 - jieyouxu:revert-broken-pipe, r=onur-ozkan

Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags"

In [rust-lang#131059] we found out that `-Zon-broken-pipe=kill` is actually **load-bearing**[^1] for
(at least) `rustc` and `rustdoc` to have the kill-process-on-broken-pipe behavior, e.g. `rustc
--print=sysroot | false` will ICE and `rustdoc --print=sysroot | false` will panic on a broken pipe.

This PR reverts 5a7058c (reverts PR rust-lang#131060) in favor of a future
fix to *unconditionally* apply `-Zon-broken-pipe=kill` to tool builds and also not drop the
`-Zon-broken-pipe=kill` flag for rustc binary builds.

I could not figure out how to write a regression test for the `rustc --print=sysroot | false`
behavior on Unix, so this is a plain revert for now.

This revert will unfortunately reintroduce rust-lang#130980 until we fix it again with the different approach.

See more details at <rust-lang#131059 (comment)> and in the timeline below.

### Timeline of kill-process-on-broken-pipe behavior changes

See [`unix_sigpipe` tracking issue rust-lang#97889][rust-lang#97889] for more context around unix sigpipe handling.

- From the very beginning since 2014, Rust binaries by default use `sig_ign`. This meant that if
  output pipe is broken yet the program tries to use `println!` and such, there will be a broken
  pipe panic from std. This lead to ICEs from e.g. `rustc --help | false` [rust-lang#34376].
- [rust-lang#49606] mitigated [rust-lang#34376] by adding an explicit signal handler to `rustc_driver` register a
  sigpipe handler with `SIG_DFL` which will cause the binary using `rustc_driver` to terminate if
  `rustc_driver::set_sigpipe_handler()` is called. `rustc`'s main binary wrapper uses
  `rustc_driver::set_sigpipe_handler()`, and so does `rustdoc`.
- A more universal way to set sigpipe behavior for Unix was introduced as part of [rust-lang#97889], i.e. `#
  [unix_sigpipe = "sig_dfl"]` attribute.
- [rust-lang#102587] migrated `rustc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`.
- [rust-lang#103495] migrated `rustdoc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`. `rustc_driver::set_sigpipe_handler` was removed.
- Following concerns about sigpipe setting UI in [rust-lang#97889], the UI for specifying sigpipe behavior
  was changed in [rust-lang#124480] from `#[unix_sigpipe = "sig_dfl"]` attribute to the commmand line flag
  `-Zon-broken-pipe=kill`.
    - In the same PR, `#[unix_sigpipe = "sig_dfl"]` were removed from `rustc` and `rustdoc` main
      binary crate entry points in favor of the command line flag. Kill-process-on-broken-pipe
      behavior was preserved by adding `-Zon-broken-pipe=kill` for `rustdoc` tool build step and
      `rustc` during compile steps.
- [rust-lang#126934] added `-Zon-broken-pipe=kill` for tool builds *except* for cargo to help with some miri
  tests because at the time the PR was written, this would lead to a couple of cargo test failures.
  Conditionally setting `RUSTFLAGS` can lead to tool build invalidation, e.g. building `cargo`
  without `-Zon-broken-pipe=kill` but `clippy` with the flag can lead to invalidation of the tool
  build cache. This is not a problem at the time, because nothing (not even miri) tests built stage
  1 cargo (all used initial cargo).
- In [rust-lang#130634] we found out that `run-make` tests like `compiler-builtins` needed stage 1 cargo, not
  just beta bootstrap cargo, because there can be changes that are present in stage 1 cargo but
  absent in beta cargo, which was blocking a beta backport.
- [rust-lang#130642] and later [rust-lang#130739] now build stage 1 cargo. And as previously mentioned, since
  `-Zon-broken-pipe=kill` was specifically *not* set for cargo, this caused tool build cache
  invalidation meaning rebuilds of stage 1 even if nothing in source was changed due to differing
  `RUSTFLAGS` since `run-make` also builds `rustdoc` and such [rust-lang#130980].

[rust-lang#34376]: rust-lang#34376
[rust-lang#49606]: rust-lang#49606
[rust-lang#97889]: rust-lang#97889
[rust-lang#102587]: rust-lang#102587
[rust-lang#103495]: rust-lang#103495
[rust-lang#124480]: rust-lang#124480
[rust-lang#130634]: rust-lang#130634
[rust-lang#130642]: rust-lang#130642
[rust-lang#130739]: rust-lang#130739
[rust-lang#130980]: rust-lang#130980
[rust-lang#131059]: rust-lang#131059
[^1]: rust-lang#131059 (comment)

r? ``@onur-ozkan`` (or bootstrap)
RalfJung pushed a commit to RalfJung/miri that referenced this issue Oct 3, 2024
Revert #131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags"

In [#131059] we found out that `-Zon-broken-pipe=kill` is actually **load-bearing**[^1] for
(at least) `rustc` and `rustdoc` to have the kill-process-on-broken-pipe behavior, e.g. `rustc
--print=sysroot | false` will ICE and `rustdoc --print=sysroot | false` will panic on a broken pipe.

This PR reverts 5a7058c5a542ec42d1fa9b524f7b4f7d6845d1e9 (reverts PR #131060) in favor of a future
fix to *unconditionally* apply `-Zon-broken-pipe=kill` to tool builds and also not drop the
`-Zon-broken-pipe=kill` flag for rustc binary builds.

I could not figure out how to write a regression test for the `rustc --print=sysroot | false`
behavior on Unix, so this is a plain revert for now.

This revert will unfortunately reintroduce #130980 until we fix it again with the different approach.

See more details at <rust-lang/rust#131059 (comment)> and in the timeline below.

### Timeline of kill-process-on-broken-pipe behavior changes

See [`unix_sigpipe` tracking issue #97889][#97889] for more context around unix sigpipe handling.

- From the very beginning since 2014, Rust binaries by default use `sig_ign`. This meant that if
  output pipe is broken yet the program tries to use `println!` and such, there will be a broken
  pipe panic from std. This lead to ICEs from e.g. `rustc --help | false` [#34376].
- [#49606] mitigated [#34376] by adding an explicit signal handler to `rustc_driver` register a
  sigpipe handler with `SIG_DFL` which will cause the binary using `rustc_driver` to terminate if
  `rustc_driver::set_sigpipe_handler()` is called. `rustc`'s main binary wrapper uses
  `rustc_driver::set_sigpipe_handler()`, and so does `rustdoc`.
- A more universal way to set sigpipe behavior for Unix was introduced as part of [#97889], i.e. `#
  [unix_sigpipe = "sig_dfl"]` attribute.
- [#102587] migrated `rustc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`.
- [#103495] migrated `rustdoc` to use `#[unix_sigpipe = "sig_dfl"]` instead of
  `rustc_driver::set_sigpipe_handler`. `rustc_driver::set_sigpipe_handler` was removed.
- Following concerns about sigpipe setting UI in [#97889], the UI for specifying sigpipe behavior
  was changed in [#124480] from `#[unix_sigpipe = "sig_dfl"]` attribute to the commmand line flag
  `-Zon-broken-pipe=kill`.
    - In the same PR, `#[unix_sigpipe = "sig_dfl"]` were removed from `rustc` and `rustdoc` main
      binary crate entry points in favor of the command line flag. Kill-process-on-broken-pipe
      behavior was preserved by adding `-Zon-broken-pipe=kill` for `rustdoc` tool build step and
      `rustc` during compile steps.
- [#126934] added `-Zon-broken-pipe=kill` for tool builds *except* for cargo to help with some miri
  tests because at the time the PR was written, this would lead to a couple of cargo test failures.
  Conditionally setting `RUSTFLAGS` can lead to tool build invalidation, e.g. building `cargo`
  without `-Zon-broken-pipe=kill` but `clippy` with the flag can lead to invalidation of the tool
  build cache. This is not a problem at the time, because nothing (not even miri) tests built stage
  1 cargo (all used initial cargo).
- In [#130634] we found out that `run-make` tests like `compiler-builtins` needed stage 1 cargo, not
  just beta bootstrap cargo, because there can be changes that are present in stage 1 cargo but
  absent in beta cargo, which was blocking a beta backport.
- [#130642] and later [#130739] now build stage 1 cargo. And as previously mentioned, since
  `-Zon-broken-pipe=kill` was specifically *not* set for cargo, this caused tool build cache
  invalidation meaning rebuilds of stage 1 even if nothing in source was changed due to differing
  `RUSTFLAGS` since `run-make` also builds `rustdoc` and such [#130980].

[#34376]: rust-lang/rust#34376
[#49606]: rust-lang/rust#49606
[#97889]: rust-lang/rust#97889
[#102587]: rust-lang/rust#102587
[#103495]: rust-lang/rust#103495
[#124480]: rust-lang/rust#124480
[#130634]: rust-lang/rust#130634
[#130642]: rust-lang/rust#130642
[#130739]: rust-lang/rust#130739
[#130980]: rust-lang/rust#130980
[#131059]: rust-lang/rust#131059
[^1]: rust-lang/rust#131059 (comment)

r? ``@onur-ozkan`` (or bootstrap)
@RalfJung
Copy link
Member

RalfJung commented Oct 15, 2024

Turns out making this a compiler flag is also not great, at least in the context of build systems like rustc bootstrap itself that build a ton of stuff. It's non-trivial to figure out when to set and when to not set this compiler flag, and a lot of time has been spend tweaking this logic in bootstrap and it's still wrong -- leading to a whole sequence of issues of the form "when I do A and then B, bootstrap rebuilds more than it should", often caused by some difference in RUSTFLAGS, such as this flag. It makes a lot more sense to annotate this in the program that wants the behavior (e.g. an attribute of the main function, or a std API invoked early in main) than in the build system that has to build tons of different programs that may want different values for this flag.

Also conceptually, this is a library thing, not a compiler thing, so having it be a compiler flag seems odd.

See here for more.

@Enselic
Copy link
Member Author

Enselic commented Oct 25, 2024

I agree this is a library problem and not a compiler or language problem. The current solution I envision involves adding libraries named something like

libsigpipe_error-91d1108bf6191a61.rlib
libsigpipe_kill-4b832a03827ff95e.rlib

to the sysroot. Similar to how we already have

libpanic_abort-91d1108bf6191a61.rlib
libpanic_unwind-4b832a03827ff95e.rlib

So choosing how SIGPIPE will behave means choosing what library to use.

What I can't currently envision is how we could allow choosing what SIGPIPE library to use without introducing a new compiler flag like -Con-broken-pipe=..., similar to -Cpanic=... that already exist. But there might be a solution.

@RalfJung
Copy link
Member

Why does it have to be a compile-time decision at all? Why can't we have std::os::unix::set_sigpipe_behavior(...) to be invoked early in main?

@Enselic
Copy link
Member Author

Enselic commented Oct 25, 2024

Because SIGPIPE is setup before main is invoked. So there'd be no way to change SIGPIPE disposition 0 times. It would be changed two times. Which

  • matters for seccomp filters
  • does not live up up to Rust's standards as a systems programming langauge IMHO

Note that that workaround can already be used (and is already quite widely used), except the helper function is not provided by libstd.

@RalfJung
Copy link
Member

RalfJung commented Oct 25, 2024 via email

@jstarks
Copy link

jstarks commented Oct 25, 2024

Note that that workaround can already be used (and is already quite widely used), except the helper function is not provided by libstd.

No. There is no stable workaround today. The correct behavior for Unix tools that want to behave like Unix tools is to inherit the signal disposition from the parent process. Rust throws away the original disposition, and it cannot be recovered.

I still think a reasonable tradeoff is to save the original disposition and provide some way of restoring it via a function in early in main(). It's not perfect (there's a race window where an externally-injected SIGPIPE will be ignored), but it's a hell of a lot simpler than the perfect solutions. I don't think the seccomp issue is a real one; it seems perfectly reasonable to say that Rust programs must be able to call sigaction.

(I also think we should do a better job of steering users toward "inherit" in the docs. Right now, the docs recommend "kill" for programs producing textual output. This is not the default behavior for C (which is "inherit"), it is not a standard convention/recommendation for C programs (programs typically either explicitly ignore SIGPIPE or do nothing and so inherit the disposition), and it is not aligned with POSIX requirements for standard inbox tools (which requires that tools inherit the SIGPIPE disposition). I don't think Rust has any good reason to recommend against existing convention here.)

@bjorn3
Copy link
Member

bjorn3 commented Oct 27, 2024

What about having libstd mask off SIGPIPE by default rather than disabling is entirely and then provide a function to restore the SIGPIPE mask to the original value? If there is a SIGPIPE injected before the restore function is called, it will still trigger once SIGPIPE is unmasked by the restore function. All tools that want to be compatible with the unix behavior have to do then is call this restore function at the top of main.

@sendittothenewts
Copy link

sendittothenewts commented Nov 4, 2024

What about having libstd mask off SIGPIPE by default rather than disabling is entirely and then provide a function to restore the SIGPIPE mask to the original value? If there is a SIGPIPE injected before the restore function is called, it will still trigger once SIGPIPE is unmasked by the restore function. All tools that want to be compatible with the unix behavior have to do then is call this restore function at the top of main.

If the restorer is not called, then SIGPIPE would remain masked indefinitely, which would be strange. The mask would be inherited across fork and exec, thus exporting this strangeness to child processes, even non-Rust ones.

Also, any Rust program currently working around this issue by calling signal(SIGPIPE, SIG_DFL) would be re-broken by this.

@bjorn3
Copy link
Member

bjorn3 commented Nov 4, 2024

The mask would be inherited across fork and exec, thus exporting this strangeness to child processes, even non-Rust ones.

That also happens with disabling the signal rather than masking it, right?

@sendittothenewts
Copy link

The mask would be inherited across fork and exec, thus exporting this strangeness to child processes, even non-Rust ones.

That also happens with disabling the signal rather than masking it, right?

Yes, I didn't mean to imply that signal dispositions are not inherited. Rather, by switching to masking SIGPIPE instead of setting its disposition to SIG_IGN, the child will still inherit the block, but now in an observably different and more unusual way, which may require the (potentially non-Rust) child to handle the situation differently. For example, using sigaction to install a custom signal handler or reset it to SIG_DFL would no longer work properly. And even if the child does go the extra mile and unmask the signal, it now risks getting killed by a SIGPIPE that was triggered by its parent and has already been duly handled as EPIPE.

I'd say that @jstarks has already put forward the correct, idiomatically POSIX solution in #97889 (comment)

@jieyouxu jieyouxu added S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. S-tracking-needs-deep-research Status: This feature needs deep research to solve design or implementation issues. S-tracking-needs-documentation Status: Needs documentation. labels Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-needs-deep-research Status: This feature needs deep research to solve design or implementation issues. S-tracking-needs-documentation Status: Needs documentation. S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants