From 6ca165b38156ca71132a006cf6b4433da026e3b5 Mon Sep 17 00:00:00 2001 From: Ruslan Sayfutdinov Date: Thu, 26 May 2022 02:48:30 -0700 Subject: [PATCH] don't inherit handles by buckd on Windows Summary: When test runner waits for `buck2` process it hangs because buckd process is still running. This happens because buckd inherits handles from parent process and wait on the parent process awaits for these handles to be closed. Example: https://stackoverflow.com/questions/13243807/popen-waiting-for-child-process-even-when-the-immediate-child-has-terminated `Popen` in Python has `close_fds` argument to solve this: > On Windows, if close_fds is true then no handles will be inherited by the child process unless explicitly passed in the handle_list element of STARTUPINFO.lpAttributeList, or by standard handle redirection. https://docs.python.org/3/library/subprocess.html Rust sets `bInheritHandles` to `TRUE` unconditionally: https://github.com/rust-lang/rust/blob/acb5c16fa8acf7fd3b48fc218881f006577bab1a/library/std/src/sys/windows/process.rs#L327 There are several open issues related to this: - https://github.com/rust-lang/rust/issues/54760 - https://github.com/rust-lang/rust/issues/38227 Because of that we have to call `CreateProcessW` ourselves. Implementation is inspired by: - std library: https://github.com/rust-lang/rust/blob/master/library/std/src/sys/windows/process.rs - subprocess library: https://github.com/hniksic/rust-subprocess/blob/master/src/win32.rs - docs for `CreateProcessW`: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw Reviewed By: stepancheg Differential Revision: D36663797 fbshipit-source-id: 14431002a763042f9c160b8a7ff7f8a62dd6befa --- cli/src/commands/daemon.rs | 203 +++++++++++++++++++++++++++++++------ 1 file changed, 170 insertions(+), 33 deletions(-) diff --git a/cli/src/commands/daemon.rs b/cli/src/commands/daemon.rs index 0d986e88c351..877733e100a5 100644 --- a/cli/src/commands/daemon.rs +++ b/cli/src/commands/daemon.rs @@ -15,10 +15,11 @@ use std::{fs::File, io::Write, path::Path, process, time::Duration}; use anyhow::Context as _; use buck2_common::memory; -use buck2_core::env_helper::EnvHelper; +use buck2_core::{ + env_helper::EnvHelper, + fs::paths::{AbsPath, ForwardRelativePath}, +}; use cli_proto::DaemonProcessInfo; -#[cfg(unix)] -use daemonize::Daemonize; use dice::cycles::DetectCycles; use futures::{ channel::mpsc::{self, UnboundedSender}, @@ -241,9 +242,13 @@ impl DaemonCommand { pub fn exec(self, _matches: &clap::ArgMatches, ctx: CommandContext) -> anyhow::Result<()> { let project_root = ctx.paths()?.project_root(); let daemon_dir = ctx.paths()?.daemon_dir()?; - let stdout_path = daemon_dir.join("buckd.stdout"); - let stderr_path = daemon_dir.join("buckd.stderr"); - let pid_path = daemon_dir.join("buckd.pid"); + let stdout_path = daemon_dir.join_unnormalized(ForwardRelativePath::new("buckd.stdout")?); + let stderr_path = daemon_dir.join_unnormalized(ForwardRelativePath::new("buckd.stderr")?); + let pid_path = daemon_dir.join_unnormalized(ForwardRelativePath::new("buckd.pid")?); + + if !daemon_dir.is_dir() { + std::fs::create_dir_all(daemon_dir)?; + } let stdout = File::create(stdout_path)?; let stderr = File::create(stderr_path)?; @@ -253,33 +258,9 @@ impl DaemonCommand { std::env::set_current_dir(project_root)?; self.redirect_output(stdout, stderr)?; } else { - #[cfg(unix)] - { - // TODO(cjhopman): Daemonize is pretty un-maintained. We may need to move - // to something else or just do it ourselves. - let daemonize = Daemonize::new() - .pid_file(pid_path) - .chown_pid_file(true) - .working_directory(project_root) - // This umask corresponds to a default of `rwxr-xr-x` (which is the default on Linux). - // We should probably just leave this alone, but the Daemonize crate doesn't let us do - // that. - .umask(0o022) - .stdout(stdout) - .stderr(stderr); - daemonize.start()?; - } - #[cfg(windows)] - { - use std::os::windows::process::CommandExt; - // Restart current process in detached mode with '--dont-daemonize' flag. - let mut cmd = std::process::Command::new( - std::env::current_exe().expect("somehow couldn't get current exe"), - ); - cmd.creation_flags(winapi::um::winbase::DETACHED_PROCESS); - cmd.args(std::env::args().skip(1)); - cmd.arg("--dont-daemonize"); - cmd.spawn()?; + self.daemonize(project_root, &pid_path, stdout, stderr)?; + // On Windows process will be restarted. + if cfg!(windows) { return Ok(()); } } @@ -316,4 +297,160 @@ impl DaemonCommand { } Ok(()) } + + #[cfg(unix)] + fn daemonize( + &self, + project_root: &AbsPath, + pid_path: &AbsPath, + stdout: File, + stderr: File, + ) -> anyhow::Result<()> { + // TODO(cjhopman): Daemonize is pretty un-maintained. We may need to move + // to something else or just do it ourselves. + let daemonize = daemonize::Daemonize::new() + .pid_file(pid_path) + .chown_pid_file(true) + .working_directory(project_root) + // This umask corresponds to a default of `rwxr-xr-x` (which is the default on Linux). + // We should probably just leave this alone, but the Daemonize crate doesn't let us do + // that. + .umask(0o022) + .stdout(stdout) + .stderr(stderr); + daemonize.start()?; + Ok(()) + } + + #[cfg(windows)] + /// Restart current process in detached mode with '--dont-daemonize' flag. + fn daemonize( + &self, + project_root: &AbsPath, + _pid_path: &AbsPath, + _stdout: File, + _stderr: File, + ) -> anyhow::Result<()> { + // We have to call CreateProcessW manually because std::process::Command + // doesn't allow to set 'bInheritHandles' to false. Without this waiting on + // parent process will also wait on inherited handles of daemon process. + use std::{ + env, + ffi::{OsStr, OsString}, + iter, mem, + os::windows::ffi::OsStrExt, + ptr, + }; + + use winapi::{ + shared::minwindef::{DWORD, FALSE}, + um::{ + handleapi::CloseHandle, + processthreadsapi::{CreateProcessW, PROCESS_INFORMATION, STARTUPINFOW}, + winbase::{CREATE_NEW_PROCESS_GROUP, CREATE_UNICODE_ENVIRONMENT, DETACHED_PROCESS}, + }, + }; + + fn to_nullterm(s: &OsStr) -> Vec { + s.encode_wide().chain(iter::once(0)).collect() + } + + // Translated from ArgvQuote at http://tinyurl.com/zmgtnls + fn append_quoted(arg: &OsStr, cmdline: &mut Vec) { + if !arg.is_empty() + && !arg.encode_wide().any(|c| { + c == ' ' as u16 + || c == '\t' as u16 + || c == '\n' as u16 + || c == '\x0b' as u16 + || c == '\"' as u16 + }) + { + cmdline.extend(arg.encode_wide()); + return; + } + cmdline.push('"' as u16); + + let arg: Vec<_> = arg.encode_wide().collect(); + let mut i = 0; + while i < arg.len() { + let mut num_backslashes = 0; + while i < arg.len() && arg[i] == '\\' as u16 { + i += 1; + num_backslashes += 1; + } + + if i == arg.len() { + for _ in 0..num_backslashes * 2 { + cmdline.push('\\' as u16); + } + break; + } else if arg[i] == b'"' as u16 { + for _ in 0..num_backslashes * 2 + 1 { + cmdline.push('\\' as u16); + } + cmdline.push(arg[i]); + } else { + for _ in 0..num_backslashes { + cmdline.push('\\' as u16); + } + cmdline.push(arg[i]); + } + i += 1; + } + cmdline.push('"' as u16); + } + + #[allow(clippy::vec_init_then_push)] + fn make_command_line(program: &OsStr, args: impl Iterator) -> Vec { + let mut cmd: Vec = Vec::new(); + cmd.push(b'"' as u16); + cmd.extend(program.encode_wide()); + cmd.push(b'"' as u16); + for arg in args.map(OsString::from) { + cmd.push(b' ' as u16); + append_quoted(&arg, &mut cmd); + } + cmd.push(0); // null terminator + cmd + } + + let exe_path = env::current_exe().expect("somehow couldn't get current exe"); + let program = exe_path.as_os_str(); + let cwd = project_root.as_os_str(); + + let mut cmd = make_command_line( + program, + env::args() + .skip(1) + .chain(iter::once("--dont-daemonize".to_owned())), + ); + + let mut sinfo: STARTUPINFOW = unsafe { mem::zeroed() }; + sinfo.cb = mem::size_of::() as DWORD; + let mut pinfo: PROCESS_INFORMATION = unsafe { mem::zeroed() }; + let creation_flags = + CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_UNICODE_ENVIRONMENT; + + let status = unsafe { + CreateProcessW( + to_nullterm(program).as_ptr(), // lpApplicationName + cmd.as_mut_ptr(), // lpCommandLine + ptr::null_mut(), // lpProcessAttributes + ptr::null_mut(), // lpThreadAttributes + FALSE, // bInheritHandles + creation_flags, // dwCreationFlags + ptr::null_mut(), // lpEnvironment + to_nullterm(cwd).as_ptr(), // lpCurrentDirectory + &mut sinfo, + &mut pinfo, + ) + }; + if status == 0 { + return Err(anyhow::anyhow!(std::io::Error::last_os_error())); + } + unsafe { CloseHandle(pinfo.hThread) }; + unsafe { CloseHandle(pinfo.hProcess) }; + Ok(()) + } }