I'm a bit hesitant that sort should add the error type checks on every write, its not entirely clear how to do this but I think there should be a consistent pattern that we can abstract and use for all of the utilities.

Edit: After more investigating, this is all handled by the SIGPIPE flags internally and thats how GNU handles all of this too

Another thing to note is that the error code for GNU that gets returned is 141

CodSpeed Performance Report

Merging this PR will degrade performance by 3.19%

_{Comparing Xylphy:sort_ignore_broken_pipe (b9b61f8) with main (48a0db8)}

Summary

⚡ 4 improved benchmarks
❌ 1 regressed benchmark
✅ 277 untouched benchmarks
⏩ 38 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

This unfortunately is a very complex area and very opaque to someone new to the project but the behaviour is actually handled by the SIGPIPE flags and we can replicate all of this logic by just having:

  #[cfg(unix)]
  uucore::init_sigpipe_capture!();

  #[uucore::main]
  pub fn uumain(args: impl uucore::Args) -> UResult<()> {
      #[cfg(unix)]
      if !uucore::signals::sigpipe_was_ignored() {
          let _ = uucore::signals::enable_pipe_errors();
      }

This handles all of the logic that you are adding there and also handles if the SIGPIPE is ignore to match GNU, it also matches those correct error codes.

This would also require adding the signals feature to the cargo.toml for sort

So that approach does fix all of the issues except for one, when there's an IOFailure with sort GNU has an error code of 2, this is a pre-existing issue unrelated to the goal of fixing the broken pipes but should be made into a new issue. In this exact test scenario it only shows up with the SIGPIPE is ignored

@ChrisDryden
Thanks for the review! I already did what you suggests here.

I’m trying to add a regression test for the pipeline case (sort | head -n 0) and to assert the GNU-like status (141 in the “SIGPIPE” case).
However, using the uutests harness, child.close_stdout() doesn’t seem to trigger a broken pipe: the test still prints the full sorted output and exits 0 (so no SIGPIPE/EPIPE happens).

#[test]
#[cfg(unix)]
fn test_broken_pipe_default_exit_code_141() {
    let mut cmd = new_ucmd!();
    let mut child = cmd.args(&["ext_sort.txt"]).run_no_wait();

    // Simulate downstream closing early (like `| head -n 0`).
    child.close_stdout();

    // No diagnostic should be printed.
    child.wait().unwrap().no_stderr().code_is(141);
}

Is there a recommended way in uutests to simulate “downstream closes pipe early” so the child actually hits SIGPIPE/EPIPE? (e.g. a helper to close the read end, or a pattern other utils use.)

Happy to align the test to whatever pattern other utilities use (I couldn’t find one beyond the existing test_sigpipe_panic 😅).

Theres really not any good macros that I can find for this type of behaviour, this is how it was tested similarly in the seq utility:

#[test]
#[cfg(unix)]
fn test_broken_pipe_exits_141_no_stderr() {
    let scene = TestScenario::new(util_name!());
    let bin = scene.bin_path.clone().into_os_string();
    scene
        .cmd_shell(r#"{ seq 1 10000 | "$BIN" sort -n 2>err | head -n1; }; echo ${PIPESTATUS[1]} >code"#)
        .env("BIN", &bin)
        .succeeds();
    assert!(scene.fixtures.read("err").is_empty());
    assert_eq!(scene.fixtures.read("code").trim(), "141");
}

I would add comments as a TODO in the test that say that when the signal is trapped that the error code is returning as a 1 instead of a 2, I think that would be outside of the scope of this PR and this would already such a big improvement already

Thanks, that makes sense. I’ll push these changes shortly 😊

the current failure:

TRY 3 FAIL [   0.116s] coreutils::tests test_sort::test_broken_pipe_exits_141_no_stderr
  stdout ───

    running 1 test
    bin: "/home/runner/work/coreutils/coreutils/target/debug/coreutils"
    run: /bin/sh -c { seq 1 10000 | "$BIN" sort -n 2>err | head -n1; }; echo ${PIPESTATUS[1]} >code
    test test_sort::test_broken_pipe_exits_141_no_stderr ... FAILED

    failures:

    failures:
        test_sort::test_broken_pipe_exits_141_no_stderr

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 4622 filtered out; finished in 0.10s
    
  stderr ───

    thread 'test_sort::test_broken_pipe_exits_141_no_stderr' panicked at tests/by-util/test_sort.rs:1234:10:
    Command was expected to succeed. code: 2
    stdout = 1

     stderr = /bin/sh: 1: Bad substitution

    stack backtrace:
       0: rust_begin_unwind
                 at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/std/src/panicking.rs:692:5
       1: core::panicking::panic_fmt
                 at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/core/src/panicking.rs:75:14
       2: uutests::util::CmdResult::success
                 at ./tests/uutests/src/lib/util.rs:451:9
       3: uutests::util::UCommand::succeeds
                 at ./tests/uutests/src/lib/util.rs:2008:9
       4: tests::test_sort::test_broken_pipe_exits_141_no_stderr
                 at ./tests/by-util/test_sort.rs:1229:5
       5: tests::test_sort::test_broken_pipe_exits_141_no_stderr::{{closure}}
                 at ./tests/by-util/test_sort.rs:1226:42
       6: core::ops::function::FnOnce::call_once
                 at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/core/src/ops/function.rs:250:5
       7: core::ops::function::FnOnce::call_once
                 at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/core/src/ops/function.rs:250:5
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: test run failed

I think it's because the test is being run under /bin/sh on CI and not Bash (${PIPESTATUS[1]} is Bash-specific). I'll fix this for a bit.

GNU testsuite comparison:

Skip an intermittent issue tests/shuf/shuf-reservoir (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/sort/sort-stale-thread-mem (fails in this run but passes in the 'main' branch)

CI target doesn’t have bash in PATH (spawn fails with ENOENT), so the bash/PIPESTATUS approach isn’t portable across our jobs. I rewrote the test in Rust to simulate head -n1 by reading one line then closing the stdout pipe, and assert status.signal() == SIGPIPE (equivalent to shell 141) with empty stderr.

GNU testsuite comparison:

Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.