Keep file descriptors open by default

Author: belovdv

src/lib.rs

@@ -52,7 +52,7 @@
 //!
 //! let client = Client::new(4).expect("failed to create jobserver");
 //! let mut cmd = Command::new("make");
-//! client.configure(&mut cmd);
+//! // client.configure(&mut cmd);
 //! ```
 //!
 //! ## Caveats
@@ -188,10 +188,9 @@ impl Client {
     /// allow at most `limit` tokens to be acquired from it in parallel. More
     /// calls to `acquire` will cause the calling thread to block.
     ///
-    /// Note that the created `Client` is not automatically inherited into
-    /// spawned child processes from this program. Manual usage of the
-    /// `configure` function is required for a child process to have access to a
-    /// job server.
+    /// Note that the created `Client` is automatically inherited into
+    /// spawned child processes from this program. `disable_inheritance`
+    /// function can be used to change behavior.
     ///
     /// # Examples
     ///
@@ -219,10 +218,8 @@ impl Client {
     /// it's passing down. This function will attempt to look for these details
     /// and connect to the jobserver.
     ///
-    /// Note that the created `Client` is not automatically inherited into
-    /// spawned child processes from this program. Manual usage of the
-    /// `configure` function is required for a child process to have access to a
-    /// job server.
+    /// Note that the created `Client` is automatically inherited into
+    /// spawned child processes from this program.
     ///
     /// # Return value
     ///
@@ -238,9 +235,9 @@ impl Client {
     /// descriptors for this process and will close the file descriptors after
     /// this value is dropped.
     ///
-    /// Additionally on Unix this function will configure the file descriptors
-    /// with `CLOEXEC` so they're not automatically inherited by spawned
-    /// children.
+    /// // Additionally on Unix this function will configure the file descriptors
+    /// // with `CLOEXEC` false to ensure they're automatically inherited by spawned
+    /// // children.
     ///
     /// On unix if `check_pipe` enabled this function will check if provided
     /// files are actually pipes.
@@ -357,42 +354,13 @@ impl Client {
     /// two file descriptors for this client to be inherited to the child.
     ///
     /// On platforms other than Unix and Windows this panics.
-    pub fn configure(&self, cmd: &mut Command) {
-        cmd.env("CARGO_MAKEFLAGS", &self.mflags_env());
-        self.inner.configure(cmd);
-    }
-
-    /// Configures a child process to have access to this client's jobserver as
-    /// well.
-    ///
-    /// This function is required to be called to ensure that a jobserver is
-    /// properly inherited to a child process. If this function is *not* called
-    /// then this `Client` will not be accessible in the child process. In other
-    /// words, if not called, then `Client::from_env` will return `None` in the
-    /// child process (or the equivalent of `Child::from_env` that `make` uses).
-    ///
-    /// ## Platform-specific behavior
+    pub fn _configure(&self, _cmd: &mut Command) {}
     ///
-    /// On Unix and Windows this will clobber the `CARGO_MAKEFLAGS`,
-    /// `MAKEFLAGS` and `MFLAGS` environment variables for the child process,
-    /// and on Unix this will also allow the two file descriptors for
-    /// this client to be inherited to the child.
-    ///
-    /// On platforms other than Unix and Windows this panics.
-    pub fn configure_make(&self, cmd: &mut Command) {
-        let value = self.mflags_env();
-        cmd.env("CARGO_MAKEFLAGS", &value);
-        cmd.env("MAKEFLAGS", &value);
-        cmd.env("MFLAGS", &value);
-        self.inner.configure(cmd);
-    }
-
-    fn mflags_env(&self) -> String {
-        let arg = self.inner.string_arg();
-        // Older implementations of make use `--jobserver-fds` and newer
-        // implementations use `--jobserver-auth`, pass both to try to catch
-        // both implementations.
-        format!("-j --jobserver-fds={0} --jobserver-auth={0}", arg)
+    pub fn disable_inheritance(&self, cmd: &mut Command) {
+        cmd.env_remove("CARGO_MAKEFLAGS");
+        cmd.env_remove("MAKEFLAGS");
+        cmd.env_remove("MFLAGS");
+        self.inner.disable_inheritance(cmd);
     }
 
     /// Converts this `Client` into a helper thread to deal with a blocking

src/unix.rs

@@ -28,7 +28,7 @@ pub struct Acquired {
 
 impl Client {
     pub fn new(mut limit: usize) -> io::Result<Client> {
-        let client = unsafe { Client::mk()? };
+        let (client, string_arg) = unsafe { Client::mk()? };
 
         // I don't think the character written here matters, but I could be
         // wrong!
@@ -47,39 +47,21 @@ impl Client {
 
         set_nonblocking(write.as_raw_fd(), false)?;
 
+        // TODO: what if this var has already been set?
+        std::env::set_var("CARGO_MAKEFLAGS", string_arg);
+
         Ok(client)
     }
 
-    unsafe fn mk() -> io::Result<Client> {
+    unsafe fn mk() -> io::Result<(Client, String)> {
         let mut pipes = [0; 2];
-
-        // Attempt atomically-create-with-cloexec if we can on Linux,
-        // detected by using the `syscall` function in `libc` to try to work
-        // with as many kernels/glibc implementations as possible.
-        #[cfg(target_os = "linux")]
-        {
-            use std::sync::atomic::{AtomicBool, Ordering};
-
-            static PIPE2_AVAILABLE: AtomicBool = AtomicBool::new(true);
-            if PIPE2_AVAILABLE.load(Ordering::SeqCst) {
-                match libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC) {
-                    -1 => {
-                        let err = io::Error::last_os_error();
-                        if err.raw_os_error() == Some(libc::ENOSYS) {
-                            PIPE2_AVAILABLE.store(false, Ordering::SeqCst);
-                        } else {
-                            return Err(err);
-                        }
-                    }
-                    _ => return Ok(Client::from_fds(pipes[0], pipes[1])),
-                }
-            }
-        }
-
         cvt(libc::pipe(pipes.as_mut_ptr()))?;
-        drop(set_cloexec(pipes[0], true));
-        drop(set_cloexec(pipes[1], true));
-        Ok(Client::from_fds(pipes[0], pipes[1]))
+        let string_arg = format!(
+            "--jobserver-fds={},{}",
+            pipes[0].as_raw_fd(),
+            pipes[1].as_raw_fd()
+        );
+        Ok((Client::from_fds(pipes[0], pipes[1]), string_arg))
     }
 
     pub(crate) unsafe fn open(s: &str, check_pipe: bool) -> Result<Client, FromEnvErrorInner> {
@@ -149,8 +131,10 @@ impl Client {
             }
         }
 
-        drop(set_cloexec(read, true));
-        drop(set_cloexec(write, true));
+        // // Unless env var was explicitly removed, file descriptors should be passed.
+        // drop(set_cloexec(read, false));
+        // drop(set_cloexec(write, false));
+
         Ok(Some(Client::from_fds(read, write)))
     }
 
@@ -263,38 +247,26 @@ impl Client {
         }
     }
 
-    pub fn string_arg(&self) -> String {
-        match self {
-            Client::Pipe { read, write } => format!("{},{}", read.as_raw_fd(), write.as_raw_fd()),
-            Client::Fifo { path, .. } => format!("fifo:{}", path.to_str().unwrap()),
-        }
-    }
-
     pub fn available(&self) -> io::Result<usize> {
         let mut len = MaybeUninit::<c_int>::uninit();
         cvt(unsafe { libc::ioctl(self.read().as_raw_fd(), libc::FIONREAD, len.as_mut_ptr()) })?;
         Ok(unsafe { len.assume_init() } as usize)
     }
 
-    pub fn configure(&self, cmd: &mut Command) {
-        match self {
-            // We `File::open`ed it when inheriting from environment,
-            // so no need to set cloexec for fifo.
-            Client::Fifo { .. } => return,
-            Client::Pipe { .. } => {}
-        };
-        // Here we basically just want to say that in the child process
-        // we'll configure the read/write file descriptors to *not* be
-        // cloexec, so they're inherited across the exec and specified as
-        // integers through `string_arg` above.
-        let read = self.read().as_raw_fd();
-        let write = self.write().as_raw_fd();
-        unsafe {
-            cmd.pre_exec(move || {
-                set_cloexec(read, false)?;
-                set_cloexec(write, false)?;
-                Ok(())
-            });
+    pub fn disable_inheritance(&self, cmd: &mut Command) {
+        if let Client::Pipe { .. } = self {
+            // Here we basically just want to say that in the child process
+            // we'll configure the read/write file descriptors to be cloexec,
+            // so they aren't inherited across the exec.
+            let read = self.read().as_raw_fd();
+            let write = self.write().as_raw_fd();
+            unsafe {
+                cmd.pre_exec(move || {
+                    set_cloexec(read, true)?;
+                    set_cloexec(write, true)?;
+                    Ok(())
+                });
+            }
         }
     }
 }

src/wasm.rs

@@ -53,19 +53,12 @@ impl Client {
         Ok(())
     }
 
-    pub fn string_arg(&self) -> String {
-        panic!(
-            "On this platform there is no cross process jobserver support,
-             so Client::configure is not supported."
-        );
-    }
-
     pub fn available(&self) -> io::Result<usize> {
         let lock = self.inner.count.lock().unwrap_or_else(|e| e.into_inner());
         Ok(*lock)
     }
 
-    pub fn configure(&self, _cmd: &mut Command) {
+    pub fn disable_inheritance(&self, _cmd: &mut Command) {
         unreachable!();
     }
 }

src/windows.rs

@@ -96,7 +96,7 @@ impl Client {
             unsafe {
                 let create_limit = if limit == 0 { 1 } else { limit };
                 let r = CreateSemaphoreA(
-                    ptr::null_mut(),
+                    ptr::null_mut(), // SECURITY_ATTRIBUTES::bInheritHandle.
                     create_limit as LONG,
                     create_limit as LONG,
                     name.as_ptr() as *const _,
@@ -118,6 +118,9 @@ impl Client {
                 if create_limit != limit {
                     client.acquire()?;
                 }
+
+                let string_arg = format!("--jobserver-auth={}", client.name);
+                std::env::set_var("CARGO_MAKEFLAGS", string_arg);
                 return Ok(client);
             }
         }
@@ -170,10 +173,6 @@ impl Client {
         }
     }
 
-    pub fn string_arg(&self) -> String {
-        self.name.clone()
-    }
-
     pub fn available(&self) -> io::Result<usize> {
         // Can't read value of a semaphore on Windows, so
         // try to acquire without sleeping, since we can find out the
@@ -194,10 +193,7 @@ impl Client {
         }
     }
 
-    pub fn configure(&self, _cmd: &mut Command) {
-        // nothing to do here, we gave the name of our semaphore to the
-        // child above
-    }
+    pub fn disable_inheritance(&self, _cmd: &mut Command) {}
 }
 
 #[derive(Debug)]

tests/client-of-myself.rs

@@ -29,7 +29,7 @@ fn server() {
     let client = t!(Client::new(1));
     let mut cmd = Command::new(me);
     cmd.env("I_AM_THE_CLIENT", "1").stdout(Stdio::piped());
-    client.configure(&mut cmd);
+    // client.configure(&mut cmd);
     let acq = client.acquire().unwrap();
     let mut child = t!(cmd.spawn());
     let stdout = child.stdout.take().unwrap();
@@ -52,7 +52,8 @@ fn server() {
 }
 
 fn client() {
-    let client = unsafe { Client::from_env().unwrap() };
+    let client = unsafe { Client::from_env_ext(true) }.client.unwrap();
+    // let client = unsafe { Client::from_env().unwrap() };
     let acq = client.acquire().unwrap();
     println!("hello!");
     drop(acq);

tests/make-as-a-client.rs

@@ -35,7 +35,7 @@ fn main() {
         return;
     }
 
-    let c = t!(Client::new(1));
+    let _c = t!(Client::new(1));
     let td = tempfile::tempdir().unwrap();
 
     let prog = env::var("MAKE").unwrap_or_else(|_| "make".to_string());
@@ -64,7 +64,7 @@ bar:
 
     // We're leaking one extra token to `make` sort of violating the makefile
     // jobserver protocol. It has the desired effect though.
-    c.configure(&mut cmd);
+    // c.configure(&mut cmd);
 
     let listener = t!(TcpListener::bind("127.0.0.1:0"));
     let addr = t!(listener.local_addr());

tests/server.rs

@@ -99,7 +99,7 @@ bar:
     // token", so we acquire our one token to drain the jobserver, and this
     // should mean that `make` itself never has a second token available to it.
     let _a = c.acquire();
-    c.configure(&mut cmd);
+    // c.configure(&mut cmd);
     let output = t!(cmd.output());
     println!(
         "\n\t=== stderr\n\t\t{}",
@@ -132,7 +132,7 @@ foo
 
 #[test]
 fn make_as_a_multi_thread_client() {
-    let c = t!(Client::new(1));
+    let _c = t!(Client::new(1));
     let td = tempfile::tempdir().unwrap();
 
     let prog = env::var("MAKE").unwrap_or_else(|_| "make".to_string());
@@ -151,7 +151,7 @@ bar:
 
     // We're leaking one extra token to `make` sort of violating the makefile
     // jobserver protocol. It has the desired effect though.
-    c.configure(&mut cmd);
+    // c.configure(&mut cmd);
     let output = t!(cmd.output());
     println!(
         "\n\t=== stderr\n\t\t{}",