From bbe0ede68711bda95dd132e15656f73a294c2a2c Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 6 Jul 2022 23:25:03 +1000 Subject: [PATCH 01/18] Fix `unix::Client::configure`: Leak `Arc` Signed-off-by: Jiahao XU --- src/unix.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 2373fb7..8fcf628 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -204,17 +204,24 @@ impl Client { format!("{},{}", self.read.as_raw_fd(), self.write.as_raw_fd()) } - pub fn configure(&self, cmd: &mut Command) { + pub fn configure(self: &Arc, cmd: &mut Command) { + // Since this process will call exec and replace the existing + // memory image, we can leak self without worrying about + // resource exhaustion. + let this = mem::ManuallyDrop::new(Arc::clone(self)); + // 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 || { + let read = this.read.as_raw_fd(); + let write = this.write.as_raw_fd(); + set_cloexec(read, false)?; set_cloexec(write, false)?; + Ok(()) }); } From b67340f72ed93dd1740d4f5743830e647f785e81 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Fri, 8 Jul 2022 01:20:57 +1000 Subject: [PATCH 02/18] Fix `unix::Client::configure`: Avoid leaking in parent Since `ManuallyDrop>` would also leak the `Arc` in parent, switch to using `Arc::increment_strong_count` instead. Signed-off-by: Jiahao XU --- src/unix.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 8fcf628..9ba3c46 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -205,26 +205,31 @@ impl Client { } pub fn configure(self: &Arc, cmd: &mut Command) { - // Since this process will call exec and replace the existing - // memory image, we can leak self without worrying about - // resource exhaustion. - let this = mem::ManuallyDrop::new(Arc::clone(self)); + let this = Arc::clone(self); // 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. - unsafe { - cmd.pre_exec(move || { - let read = this.read.as_raw_fd(); - let write = this.write.as_raw_fd(); - - set_cloexec(read, false)?; - set_cloexec(write, false)?; + let f = move || { + let p = Arc::as_ptr(&this); + // Increment strong count to ensure that it is leaked. + // + // Safety: + // + // p must be a valid pointer to Arc. + unsafe { Arc::increment_strong_count(p) }; + + let read = this.read.as_raw_fd(); + let write = this.write.as_raw_fd(); + + set_cloexec(read, false)?; + set_cloexec(write, false)?; + + Ok(()) + }; - Ok(()) - }); - } + unsafe { cmd.pre_exec(f) }; } } From fb3900912dc0225ccf91624d8f1bdd038f2317c9 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Fri, 8 Jul 2022 01:28:20 +1000 Subject: [PATCH 03/18] Fix safety issue with `unix::Client::configure` Signed-off-by: Jiahao XU --- src/unix.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index 9ba3c46..eae218b 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -206,19 +206,26 @@ impl Client { pub fn configure(self: &Arc, cmd: &mut Command) { let this = Arc::clone(self); + let p = ShareablePtr(Arc::into_raw(this)); + + // Safety: + // + // p.0 is a pointer obtained through Arc::into_raw + // and the associated Arc is valid. + let this = unsafe { Arc::from_raw(p.0) }; // 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 f = move || { - let p = Arc::as_ptr(&this); // Increment strong count to ensure that it is leaked. // // Safety: // - // p must be a valid pointer to Arc. - unsafe { Arc::increment_strong_count(p) }; + // p.0 is a pointer obtained through Arc::into_raw + // and the associated Arc is valid. + unsafe { Arc::increment_strong_count(p.0) }; let read = this.read.as_raw_fd(); let write = this.write.as_raw_fd(); @@ -371,3 +378,8 @@ extern "C" fn sigusr1_handler( ) { // nothing to do } + +struct ShareablePtr(*const T); + +unsafe impl Send for ShareablePtr {} +unsafe impl Sync for ShareablePtr {} From cf5604034ead0e0e042552c91f9d39359be678c4 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Fri, 8 Jul 2022 01:45:00 +1000 Subject: [PATCH 04/18] Improve doc in `unix::Client::configure` Signed-off-by: Jiahao XU --- src/unix.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/unix.rs b/src/unix.rs index eae218b..e3918ab 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -221,6 +221,14 @@ impl Client { let f = move || { // Increment strong count to ensure that it is leaked. // + // Basically, Arc uses reference counting to keep track of the + // references and it is dropped when the reference counting + // reaches 0. + // + // Using Arc::increment_strong_count increment the reference + // counting by 1 without creating an Arc, thus prevents the + // reference counting from ever reaching 0 and being dropped. + // // Safety: // // p.0 is a pointer obtained through Arc::into_raw From 1a41499b85a76f214214627ba9de077da44921a9 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 17 Jul 2022 21:42:01 +1000 Subject: [PATCH 05/18] Simplify `unix::Client::configure` impl Since `Command` keeps pre exec function alive, we just need to pass in an `Arc` Signed-off-by: Jiahao XU --- src/unix.rs | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index e3918ab..02db7bb 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -206,35 +206,12 @@ impl Client { pub fn configure(self: &Arc, cmd: &mut Command) { let this = Arc::clone(self); - let p = ShareablePtr(Arc::into_raw(this)); - - // Safety: - // - // p.0 is a pointer obtained through Arc::into_raw - // and the associated Arc is valid. - let this = unsafe { Arc::from_raw(p.0) }; // 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 f = move || { - // Increment strong count to ensure that it is leaked. - // - // Basically, Arc uses reference counting to keep track of the - // references and it is dropped when the reference counting - // reaches 0. - // - // Using Arc::increment_strong_count increment the reference - // counting by 1 without creating an Arc, thus prevents the - // reference counting from ever reaching 0 and being dropped. - // - // Safety: - // - // p.0 is a pointer obtained through Arc::into_raw - // and the associated Arc is valid. - unsafe { Arc::increment_strong_count(p.0) }; - let read = this.read.as_raw_fd(); let write = this.write.as_raw_fd(); @@ -244,6 +221,8 @@ impl Client { Ok(()) }; + // cmd will store `f` until it is dropped. + // Thus, the Arc inside `f` will also be kept alive. unsafe { cmd.pre_exec(f) }; } } @@ -386,8 +365,3 @@ extern "C" fn sigusr1_handler( ) { // nothing to do } - -struct ShareablePtr(*const T); - -unsafe impl Send for ShareablePtr {} -unsafe impl Sync for ShareablePtr {} From acf47a31444be340769da12849c012bafc51350a Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 17 Jul 2022 21:57:21 +1000 Subject: [PATCH 06/18] Add test client-dropped-before-command.rs Signed-off-by: Jiahao XU --- tests/client-dropped-before-command.rs | 37 ++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 tests/client-dropped-before-command.rs diff --git a/tests/client-dropped-before-command.rs b/tests/client-dropped-before-command.rs new file mode 100644 index 0000000..04a938a --- /dev/null +++ b/tests/client-dropped-before-command.rs @@ -0,0 +1,37 @@ +use std::env; +use std::process::{Command, Output}; + +use jobserver::Client; + +fn main() { + if env::var("I_AM_THE_CLIENT").is_ok() { + client(); + } else { + server(); + } +} + +fn server() { + let me = env::current_exe().unwrap(); + let client = Client::new(1).unwrap(); + + let mut cmd = Command::new(me); + cmd.env("I_AM_THE_CLIENT", "1"); + client.configure(&mut cmd); + + let Output { + status, + stdout: _stdout, + stderr, + } = cmd.output().unwrap(); + + assert!(status.success()); + assert_eq!(&*stderr, b"hello!"); +} + +fn client() { + let client = unsafe { Client::from_env().unwrap() }; + let acq = client.acquire().unwrap(); + eprintln!("hello!"); + drop(acq); +} From fc8bc57727bdef6bd08268d8a147c1a858719039 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Mon, 18 Jul 2022 23:54:04 +1000 Subject: [PATCH 07/18] Fix client-dropped-before-command: `drop(client)` Signed-off-by: Jiahao XU --- tests/client-dropped-before-command.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/client-dropped-before-command.rs b/tests/client-dropped-before-command.rs index 04a938a..f18f7a6 100644 --- a/tests/client-dropped-before-command.rs +++ b/tests/client-dropped-before-command.rs @@ -18,6 +18,7 @@ fn server() { let mut cmd = Command::new(me); cmd.env("I_AM_THE_CLIENT", "1"); client.configure(&mut cmd); + drop(client); let Output { status, From 1ca91b7424c2e7fcc522351df7c44bccace7cc1b Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Mon, 18 Jul 2022 23:58:05 +1000 Subject: [PATCH 08/18] Fix `Cargo.toml`: Run client_dropped_before_command Signed-off-by: Jiahao XU --- Cargo.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index b158c8d..debf43f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,6 +35,11 @@ name = "client-of-myself" path = "tests/client-of-myself.rs" harness = false +[[test]] +name = "client-dropped-before-command" +path = "tests/client-dropped-before-command.rs" +harness = false + [[test]] name = "make-as-a-client" path = "tests/make-as-a-client.rs" From c01444ad9df7c447ee640b78acd434a8f05db327 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Mon, 18 Jul 2022 23:58:15 +1000 Subject: [PATCH 09/18] Fix `assert_eq!` in `client-dropped-before-command` Signed-off-by: Jiahao XU --- tests/client-dropped-before-command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/client-dropped-before-command.rs b/tests/client-dropped-before-command.rs index f18f7a6..b7f1175 100644 --- a/tests/client-dropped-before-command.rs +++ b/tests/client-dropped-before-command.rs @@ -27,7 +27,7 @@ fn server() { } = cmd.output().unwrap(); assert!(status.success()); - assert_eq!(&*stderr, b"hello!"); + assert_eq!(&*stderr, b"hello!\n"); } fn client() { From ccf581bcbbe2ef9ba1b4fb010f106c6e97f8801a Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Tue, 19 Jul 2022 00:03:33 +1000 Subject: [PATCH 10/18] Improve err msg of `client-dropped-before-command` Signed-off-by: Jiahao XU --- tests/client-dropped-before-command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/client-dropped-before-command.rs b/tests/client-dropped-before-command.rs index b7f1175..846065f 100644 --- a/tests/client-dropped-before-command.rs +++ b/tests/client-dropped-before-command.rs @@ -26,7 +26,7 @@ fn server() { stderr, } = cmd.output().unwrap(); - assert!(status.success()); + assert!(status.success(), "{:#?}", String::from_utf8_lossy(&stderr)); assert_eq!(&*stderr, b"hello!\n"); } From 654da682dcfcd8660d471e7f102721fa4ab00aeb Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:26:25 +1000 Subject: [PATCH 11/18] Fix `configure` API: Change to `configure_{make_}and_run` That takes any type that implements `Command` and a function `f` that spawns the process. Signed-off-by: Jiahao XU --- src/lib.rs | 101 +++++++++++++++++++++++++++++++++++++++++++------ src/unix.rs | 27 +++++-------- src/wasm.rs | 17 +++++++-- src/windows.rs | 10 ++++- 4 files changed, 120 insertions(+), 35 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1272eaa..32fa0e2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -49,7 +49,7 @@ //! //! let client = Client::new(4).expect("failed to create jobserver"); //! let mut cmd = Command::new("make"); -//! client.configure(&mut cmd); +//! let child = client.configure_and_run(&mut cmd, |cmd| cmd.spawn()).unwrap(); //! ``` //! //! ## Caveats @@ -79,8 +79,9 @@ #![doc(html_root_url = "https://docs.rs/jobserver/0.1")] use std::env; +use std::ffi; use std::io; -use std::process::Command; +use std::process; use std::sync::{Arc, Condvar, Mutex, MutexGuard}; #[cfg(unix)] @@ -93,6 +94,46 @@ mod imp; #[path = "wasm.rs"] mod imp; +/// Command that can be accepted by this crate. +pub trait Command { + /// Inserts or updates an environment variable mapping. + fn env(&mut self, key: K, val: V) -> &mut Self + where + K: AsRef, + V: AsRef; + + /// Removes an environment variable mapping. + fn env_remove>(&mut self, key: K) -> &mut Self; +} +impl Command for process::Command { + fn env(&mut self, key: K, val: V) -> &mut Self + where + K: AsRef, + V: AsRef, + { + process::Command::env(self, key.as_ref(), val.as_ref()) + } + + fn env_remove>(&mut self, key: K) -> &mut Self { + process::Command::env_remove(self, key.as_ref()) + } +} +impl Command for &mut T { + fn env(&mut self, key: K, val: V) -> &mut Self + where + K: AsRef, + V: AsRef, + { + (*self).env(key.as_ref(), val.as_ref()); + self + } + + fn env_remove>(&mut self, key: K) -> &mut Self { + (*self).env_remove(key.as_ref()); + self + } +} + /// A client of a jobserver /// /// This structure is the main type exposed by this library, and is where @@ -274,7 +315,10 @@ impl Client { } /// Configures a child process to have access to this client's jobserver as - /// well. + /// well and run the `f` which spawns the process. + /// + /// NOTE that you have to spawn the process inside `f`, otherwise the jobserver + /// would be inherited. /// /// 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 @@ -289,13 +333,25 @@ 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) { + pub fn configure_and_run(&self, mut cmd: Cmd, f: F) -> io::Result + where + Cmd: Command, + F: FnOnce(&mut Cmd) -> io::Result, + { cmd.env("CARGO_MAKEFLAGS", &self.mflags_env()); - self.inner.configure(cmd); + + let res = self.do_run(&mut cmd, f); + + cmd.env_remove("CARGO_MAKEFLAGS"); + + res } /// Configures a child process to have access to this client's jobserver as - /// well. + /// well and run the `f` which spawns the process. + /// + /// NOTE that you have to spawn the process inside `f`, otherwise the jobserver + /// would be inherited. /// /// 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 @@ -311,12 +367,35 @@ impl Client { /// 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) { + pub fn configure_make_and_run(&self, mut cmd: Cmd, f: F) -> io::Result + where + Cmd: Command, + F: FnOnce(&mut Cmd) -> io::Result, + { let value = self.mflags_env(); - cmd.env("CARGO_MAKEFLAGS", &value); - cmd.env("MAKEFLAGS", &value); - cmd.env("MFLAGS", &value); - self.inner.configure(cmd); + cmd.env("CARGO_MAKEFLAGS", &value) + .env("MAKEFLAGS", &value) + .env("MFLAGS", &value); + + let res = self.do_run(&mut cmd, f); + + cmd.env_remove("CARGO_MAKEFLAGS") + .env_remove("MAKEFLAGS") + .env_remove("MFLAGS"); + + res + } + + fn do_run(&self, cmd: &mut Cmd, f: F) -> io::Result + where + Cmd: Command, + F: FnOnce(&mut Cmd) -> io::Result, + { + self.inner.pre_run()?; + let res = f(cmd); + self.inner.post_run()?; + + res } fn mflags_env(&self) -> String { diff --git a/src/unix.rs b/src/unix.rs index 02db7bb..fe4a57c 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -4,7 +4,6 @@ use std::fs::File; use std::io::{self, Read, Write}; use std::mem; use std::os::unix::prelude::*; -use std::process::Command; use std::ptr; use std::sync::{Arc, Once}; use std::thread::{self, Builder, JoinHandle}; @@ -204,26 +203,18 @@ impl Client { format!("{},{}", self.read.as_raw_fd(), self.write.as_raw_fd()) } - pub fn configure(self: &Arc, cmd: &mut Command) { - let this = Arc::clone(self); + pub fn pre_run(&self) -> io::Result<()> { + set_cloexec(self.read.as_raw_fd(), false)?; + set_cloexec(self.write.as_raw_fd(), false)?; - // 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 f = move || { - let read = this.read.as_raw_fd(); - let write = this.write.as_raw_fd(); + Ok(()) + } - set_cloexec(read, false)?; - set_cloexec(write, false)?; + pub fn post_run(&self) -> io::Result<()> { + set_cloexec(self.read.as_raw_fd(), true)?; + set_cloexec(self.write.as_raw_fd(), true)?; - Ok(()) - }; - - // cmd will store `f` until it is dropped. - // Thus, the Arc inside `f` will also be kept alive. - unsafe { cmd.pre_exec(f) }; + Ok(()) } } diff --git a/src/wasm.rs b/src/wasm.rs index b88a9d9..c37f582 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -1,5 +1,4 @@ use std::io; -use std::process::Command; use std::sync::{Arc, Condvar, Mutex}; use std::thread::{Builder, JoinHandle}; @@ -55,12 +54,22 @@ impl Client { pub fn string_arg(&self) -> String { panic!( "On this platform there is no cross process jobserver support, - so Client::configure is not supported." + so Client::configure_and_run is not supported." ); } - pub fn configure(&self, _cmd: &mut Command) { - unreachable!(); + pub fn pre_run(&self) -> io::Result<()> { + panic!( + "On this platform there is no cross process jobserver support, + so Client::configure_and_run is not supported." + ); + } + + pub fn post_run(&self) -> io::Result<()> { + panic!( + "On this platform there is no cross process jobserver support, + so Client::configure_and_run is not supported." + ); } } diff --git a/src/windows.rs b/src/windows.rs index d795c1c..4e0a276 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -1,6 +1,5 @@ use std::ffi::CString; use std::io; -use std::process::Command; use std::ptr; use std::sync::Arc; use std::thread::{Builder, JoinHandle}; @@ -170,9 +169,16 @@ impl Client { self.name.clone() } - pub fn configure(&self, _cmd: &mut Command) { + pub fn pre_run(&self) -> io::Result<()> { // nothing to do here, we gave the name of our semaphore to the // child above + Ok(()) + } + + pub fn post_run(&self) -> io::Result<()> { + // nothing to do here, we gave the name of our semaphore to the + // child above + Ok(()) } } From 3f5028690ccfc4897e2f0912eb35e0bdf1ccf001 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:29:53 +1000 Subject: [PATCH 12/18] Update integration tests to use the new API Signed-off-by: Jiahao XU --- tests/client-dropped-before-command.rs | 6 +++--- tests/client-of-myself.rs | 7 +++++-- tests/make-as-a-client.rs | 9 ++++----- tests/server.rs | 6 ++---- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/client-dropped-before-command.rs b/tests/client-dropped-before-command.rs index 846065f..635da43 100644 --- a/tests/client-dropped-before-command.rs +++ b/tests/client-dropped-before-command.rs @@ -17,14 +17,14 @@ fn server() { let mut cmd = Command::new(me); cmd.env("I_AM_THE_CLIENT", "1"); - client.configure(&mut cmd); - drop(client); let Output { status, stdout: _stdout, stderr, - } = cmd.output().unwrap(); + } = client + .configure_and_run(&mut cmd, |cmd| cmd.output()) + .unwrap(); assert!(status.success(), "{:#?}", String::from_utf8_lossy(&stderr)); assert_eq!(&*stderr, b"hello!\n"); diff --git a/tests/client-of-myself.rs b/tests/client-of-myself.rs index 45d57b0..90fae95 100644 --- a/tests/client-of-myself.rs +++ b/tests/client-of-myself.rs @@ -29,9 +29,12 @@ 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); + let acq = client.acquire().unwrap(); - let mut child = t!(cmd.spawn()); + let mut child = client + .configure_and_run(&mut cmd, |cmd| cmd.spawn()) + .unwrap(); + let stdout = child.stdout.take().unwrap(); let (tx, rx) = mpsc::channel(); let t = thread::spawn(move || { diff --git a/tests/make-as-a-client.rs b/tests/make-as-a-client.rs index 4faac5b..0f8df41 100644 --- a/tests/make-as-a-client.rs +++ b/tests/make-as-a-client.rs @@ -62,14 +62,13 @@ bar: .as_bytes() )); - // 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); - let listener = t!(TcpListener::bind("127.0.0.1:0")); let addr = t!(listener.local_addr()); cmd.env("TEST_ADDR", addr.to_string()); - let mut child = t!(cmd.spawn()); + + // We're leaking one extra token to `make` sort of violating the makefile + // jobserver protocol. It has the desired effect though. + let mut child = c.configure_and_run(&mut cmd, |cmd| cmd.spawn()).unwrap(); // We should get both connections as the two programs should be run // concurrently. diff --git a/tests/server.rs b/tests/server.rs index c92676e..daa599b 100644 --- a/tests/server.rs +++ b/tests/server.rs @@ -75,8 +75,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); - let output = t!(cmd.output()); + let output = c.configure_and_run(&mut cmd, |cmd| cmd.output()).unwrap(); println!( "\n\t=== stderr\n\t\t{}", String::from_utf8_lossy(&output.stderr).replace("\n", "\n\t\t") @@ -127,8 +126,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); - let output = t!(cmd.output()); + let output = c.configure_and_run(&mut cmd, |cmd| cmd.output()).unwrap(); println!( "\n\t=== stderr\n\t\t{}", String::from_utf8_lossy(&output.stderr).replace("\n", "\n\t\t") From 84f8b33ff941f40427a572e2f1eb36e26ea15569 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:32:30 +1000 Subject: [PATCH 13/18] Impl `Command` for `tokio::process::Command` Signed-off-by: Jiahao XU --- Cargo.toml | 3 +++ src/lib.rs | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index debf43f..8ae1f4a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,6 +11,9 @@ An implementation of the GNU make jobserver for Rust """ edition = "2018" +[dependencies] +tokio = { version = "1", no-default-features = true, features = ["process"], optional = true } + [target.'cfg(unix)'.dependencies] libc = "0.2.50" diff --git a/src/lib.rs b/src/lib.rs index 32fa0e2..8d7909c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -118,6 +118,20 @@ impl Command for process::Command { process::Command::env_remove(self, key.as_ref()) } } +#[cfg(feature = "tokio")] +impl Command for tokio::process::Command { + fn env(&mut self, key: K, val: V) -> &mut Self + where + K: AsRef, + V: AsRef, + { + tokio::process::Command::env(self, key.as_ref(), val.as_ref()) + } + + fn env_remove>(&mut self, key: K) -> &mut Self { + tokio::process::Command::env_remove(self, key.as_ref()) + } +} impl Command for &mut T { fn env(&mut self, key: K, val: V) -> &mut Self where From f284ea1787576a3b97640885fa497b335eec61ad Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:33:16 +1000 Subject: [PATCH 14/18] Add dev-dep tokio with full feature Signed-off-by: Jiahao XU --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index 8ae1f4a..f60e207 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ libc = "0.2.50" futures = "0.1" num_cpus = "1.0" tempfile = "3" +tokio = { version = "1.20.0", features = ["full"] } tokio-core = "0.1" tokio-process = "0.2" From dbc4d03fd95c528d4fcb8626cf9e68c375cd9223 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:38:40 +1000 Subject: [PATCH 15/18] Add `tests/client-dropped-before-command-tokio.rs` Signed-off-by: Jiahao XU --- Cargo.toml | 6 +++ tests/client-dropped-before-command-tokio.rs | 46 ++++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 tests/client-dropped-before-command-tokio.rs diff --git a/Cargo.toml b/Cargo.toml index f60e207..60306c6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,12 @@ name = "client-dropped-before-command" path = "tests/client-dropped-before-command.rs" harness = false +[[test]] +name = "client-dropped-before-command-tokio" +path = "tests/client-dropped-before-command-tokio.rs" +harness = false +required-features = ["tokio"] + [[test]] name = "make-as-a-client" path = "tests/make-as-a-client.rs" diff --git a/tests/client-dropped-before-command-tokio.rs b/tests/client-dropped-before-command-tokio.rs new file mode 100644 index 0000000..9f1bbef --- /dev/null +++ b/tests/client-dropped-before-command-tokio.rs @@ -0,0 +1,46 @@ +use std::env; +use std::process::Output; +use tokio::{process::Command, runtime}; + +use jobserver::Client; + +fn main() { + if env::var("I_AM_THE_CLIENT").is_ok() { + client(); + } else { + server(); + } +} + +fn server() { + let me = env::current_exe().unwrap(); + let rt = runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + + let _guard = rt.enter(); + + let client = Client::new(1).unwrap(); + + let mut cmd = Command::new(me); + cmd.env("I_AM_THE_CLIENT", "1"); + + let Output { + status, + stdout: _stdout, + stderr, + } = client + .configure_and_run(&mut cmd, |cmd| rt.block_on(cmd.output())) + .unwrap(); + + assert!(status.success(), "{:#?}", String::from_utf8_lossy(&stderr)); + assert_eq!(&*stderr, b"hello!\n"); +} + +fn client() { + let client = unsafe { Client::from_env().unwrap() }; + let acq = client.acquire().unwrap(); + eprintln!("hello!"); + drop(acq); +} From cf6771145d91cf4f0d256b88a696f783f454b0eb Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:39:06 +1000 Subject: [PATCH 16/18] Update workflow main to run test with all features Signed-off-by: Jiahao XU --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2693f1d..81e5b19 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -29,7 +29,7 @@ jobs: - name: Install Rust (rustup) run: rustup update ${{ matrix.rust }} --no-self-update && rustup default ${{ matrix.rust }} shell: bash - - run: cargo test + - run: cargo test --all-features rustfmt: name: Rustfmt From 555d498a8437f2fcd7c41bcd718bb838c782871d Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:41:43 +1000 Subject: [PATCH 17/18] Avoid `String::clone` in `windows::Client::string_arg` By changing `{windows, wasm, unix}::Client::string_arg` to return `std::borrow::Cow<'_, str>` Signed-off-by: Jiahao XU --- src/unix.rs | 9 +++++++-- src/wasm.rs | 3 ++- src/windows.rs | 5 +++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index fe4a57c..b53587b 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -1,5 +1,6 @@ use libc::c_int; +use std::borrow::Cow; use std::fs::File; use std::io::{self, Read, Write}; use std::mem; @@ -199,8 +200,12 @@ impl Client { } } - pub fn string_arg(&self) -> String { - format!("{},{}", self.read.as_raw_fd(), self.write.as_raw_fd()) + pub fn string_arg(&self) -> Cow<'_, str> { + Cow::Owned(format!( + "{},{}", + self.read.as_raw_fd(), + self.write.as_raw_fd() + )) } pub fn pre_run(&self) -> io::Result<()> { diff --git a/src/wasm.rs b/src/wasm.rs index c37f582..b86767d 100644 --- a/src/wasm.rs +++ b/src/wasm.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::io; use std::sync::{Arc, Condvar, Mutex}; use std::thread::{Builder, JoinHandle}; @@ -51,7 +52,7 @@ impl Client { Ok(()) } - pub fn string_arg(&self) -> String { + pub fn string_arg(&self) -> Cow<'_, str> { panic!( "On this platform there is no cross process jobserver support, so Client::configure_and_run is not supported." diff --git a/src/windows.rs b/src/windows.rs index 4e0a276..7d402b7 100644 --- a/src/windows.rs +++ b/src/windows.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::ffi::CString; use std::io; use std::ptr; @@ -165,8 +166,8 @@ impl Client { } } - pub fn string_arg(&self) -> String { - self.name.clone() + pub fn string_arg(&self) -> Cow<'_, str> { + Cow::Borrowed(&self.name) } pub fn pre_run(&self) -> io::Result<()> { From 4fc1bb38d1c22e13a643ceace1e6242e64e56eba Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 24 Jul 2022 17:48:35 +1000 Subject: [PATCH 18/18] Fix a typo in doc of `Client::configure_{make_}and_run` Signed-off-by: Jiahao XU --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8d7909c..d97c296 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -332,7 +332,7 @@ impl Client { /// well and run the `f` which spawns the process. /// /// NOTE that you have to spawn the process inside `f`, otherwise the jobserver - /// would be inherited. + /// would not be inherited. /// /// 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 @@ -365,7 +365,7 @@ impl Client { /// well and run the `f` which spawns the process. /// /// NOTE that you have to spawn the process inside `f`, otherwise the jobserver - /// would be inherited. + /// would not be inherited. /// /// 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