Repository: nix-community/nixpkgs-fmt
Pull request #256 (2021-08-17 17:03:46)

Properly handle SIGPIPE
Commands that print their results to stdout should terminate on `SIGPIPE`, so that they don't produce error messages when piped into programs like `head` or `less` that might exit before the process is finished.  This is the default behaviour on Unix, but the Rust runtime ignores `SIGPIPE` by default.

For nixpkgs-fmt modes where it makes sense, re-enable the Unix default behaviour of terminating on `SIGPIPE`.  Don't do this when formatting files, because only formatting half the files if stdout was closed would be a surprising behaviour.



Patch:
diff --git a/Cargo.lock b/Cargo.lock
index ba909a9..eb74c27 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -212,9 +212,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646"
 
 [[package]]
 name = "libc"
-version = "0.2.94"
+version = "0.2.99"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "18794a8ad5b29321f790b55d93dfba91e125cb1a9edbd4f8e3150acc771c1a5e"
+checksum = "a7f823d141fe0a24df1e23b4af4e3c7ba9e5966ec514ea068c93024aa7deb765"
 
 [[package]]
 name = "log"
@@ -253,6 +253,7 @@ dependencies = [
  "clap",
  "crossbeam-channel",
  "ignore",
+ "libc",
  "rnix",
  "rowan",
  "serde_json",
diff --git a/Cargo.toml b/Cargo.toml
index 69ee3bd..0891350 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,6 +23,7 @@ smol_str = "0.1.17"
 crossbeam-channel = "0.3"
 ignore = "0.4.10"
 clap = "2.33.0"
+libc = "0.2.99"
 
 # Enable serialization support for rnix syntax trees.
 serde_json = "1.0"
diff --git a/src/main.rs b/src/main.rs
index 9ebc9e7..162b39e 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,7 +1,7 @@
 use std::{
     fmt::Write,
     fs,
-    io::{stdin, Read},
+    io::{self, stdin, Read},
     path::{Path, PathBuf},
     thread,
 };
@@ -114,10 +114,19 @@ fn parse_args() -> Result<Args> {
     Ok(Args { operation, src })
 }
 
+fn reset_sigpipe() -> io::Result<()> {
+    use libc::{signal, SIGPIPE, SIG_DFL, SIG_ERR};
+    if unsafe { signal(SIGPIPE, SIG_DFL) } == SIG_ERR {
+        return Err(io::Error::last_os_error());
+    }
+    Ok(())
+}
+
 fn try_main(args: Args) -> Result<()> {
     match args.operation {
         Operation::Fmt { write_changes, fail_on_changes } => match &args.src {
             Src::Stdin => {
+                reset_sigpipe()?;
                 let input = read_stdin_to_string()?;
                 let output = nixpkgs_fmt::reformat_string(&input);
                 let has_changes = input != output;
@@ -175,6 +184,7 @@ fn try_main(args: Args) -> Result<()> {
             }
         },
         Operation::Parse { output_format } => {
+            reset_sigpipe()?;
             let input = read_single_source(&args.src)?;
             let ast = rnix::parse(&input);
             let res = match output_format {
@@ -191,6 +201,7 @@ fn try_main(args: Args) -> Result<()> {
             print!("{}", res)
         }
         Operation::Explain => {
+            reset_sigpipe()?;
             let input = read_stdin_to_string()?;
             let output = nixpkgs_fmt::explain(&input);
             print!("{}", output);


Test patch:
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
new file mode 100644
index 0000000..0c76856
--- /dev/null
+++ b/tests/integration_tests.rs
@@ -0,0 +1,59 @@
+use std::fs::File;
+use std::path::Path;
+use std::process::{Command, Stdio};
+
+// It's an important Unix convention that text processing tools such as one might
+// use in a pipeline terminate cleanly if stdout is closed prematurely.  If grep
+// didn't do this, there'd be an error message every time somebody piped it into
+// head.  Unix processes even do this by default -- when a process writes to a
+// closed pipe, it's sent a SIGPIPE signal, which by default terminates the
+// process.  But the Rust runtime ignores SIGPIPE, so Rust programs have to go out
+// of their way to restore the default SIGPIPE behaviour (or emulate it by checking
+// for EPIPE every time they write) to be good Unix citizens.
+fn test_stdout_closed(args: &[&str]) {
+    use libc::{waitpid, SIGPIPE, WIFSIGNALED, WTERMSIG};
+    use std::io::copy;
+    use std::thread::spawn;
+
+    // Drop the child's stdout to close it.
+    let (pid, mut stdin) = {
+        let child = Command::new(env!("CARGO_BIN_EXE_nixpkgs-fmt"))
+            .args(args)
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .spawn()
+            .unwrap();
+
+        (child.id(), child.stdin.unwrap())
+    };
+
+    spawn(move || {
+        let input_path =
+            Path::new(env!("CARGO_MANIFEST_DIR")).join("test_data/binop_wrap_before.bad.nix");
+        let mut input = File::open(input_path).unwrap();
+        copy(&mut input, &mut stdin).unwrap();
+        drop(stdin)
+    });
+
+    // We have to use libc because we don't have a Child struct any more.
+    let mut wstatus = 0;
+    assert_eq!(unsafe { waitpid(pid as i32, &mut wstatus, 0) }, pid as i32);
+
+    assert!(WIFSIGNALED(wstatus));
+    assert_eq!(WTERMSIG(wstatus), SIGPIPE);
+}
+
+#[test]
+fn stdout_closed() {
+    test_stdout_closed(&[]);
+}
+
+#[test]
+fn stdout_closed_parse() {
+    test_stdout_closed(&["--parse"]);
+}
+
+#[test]
+fn stdout_closed_explain() {
+    test_stdout_closed(&["--explain"]);
+}