mirror of
https://github.com/nushell/nushell.git
synced 2024-11-22 08:23:24 +01:00
Fix intermittent test crash (#6268)
* Fix intermittent test crash * fix windows build
This commit is contained in:
parent
121b801baa
commit
555d9ee763
@ -6,7 +6,6 @@ use nu_protocol::did_you_mean;
|
||||
use nu_protocol::engine::{EngineState, Stack};
|
||||
use nu_protocol::{ast::Call, engine::Command, ShellError, Signature, SyntaxShape, Value};
|
||||
use nu_protocol::{Category, Example, ListStream, PipelineData, RawStream, Span, Spanned};
|
||||
use nu_system::ForegroundProcess;
|
||||
use pathdiff::diff_paths;
|
||||
use std::collections::HashMap;
|
||||
use std::io::{BufRead, BufReader, Write};
|
||||
@ -121,7 +120,7 @@ impl ExternalCommand {
|
||||
|
||||
let ctrlc = engine_state.ctrlc.clone();
|
||||
|
||||
let mut fg_process = ForegroundProcess::new(self.create_process(&input, false, head)?);
|
||||
let mut process = self.create_process(&input, false, head)?;
|
||||
let child;
|
||||
|
||||
#[cfg(windows)]
|
||||
@ -140,10 +139,9 @@ impl ExternalCommand {
|
||||
.iter()
|
||||
.any(|&cmd| command_name_upper == cmd);
|
||||
|
||||
match fg_process.spawn() {
|
||||
match process.spawn() {
|
||||
Err(_) => {
|
||||
let mut fg_process =
|
||||
ForegroundProcess::new(self.create_process(&input, use_cmd, head)?);
|
||||
let mut fg_process = self.create_process(&input, use_cmd, head)?;
|
||||
child = fg_process.spawn();
|
||||
}
|
||||
Ok(process) => {
|
||||
@ -154,7 +152,7 @@ impl ExternalCommand {
|
||||
|
||||
#[cfg(not(windows))]
|
||||
{
|
||||
child = fg_process.spawn()
|
||||
child = process.spawn()
|
||||
}
|
||||
|
||||
match child {
|
||||
@ -197,7 +195,7 @@ impl ExternalCommand {
|
||||
engine_state.config.use_ansi_coloring = false;
|
||||
|
||||
// if there is a string or a stream, that is sent to the pipe std
|
||||
if let Some(mut stdin_write) = child.as_mut().stdin.take() {
|
||||
if let Some(mut stdin_write) = child.stdin.take() {
|
||||
std::thread::spawn(move || {
|
||||
let input = crate::Table::run(
|
||||
&crate::Table,
|
||||
@ -238,7 +236,7 @@ impl ExternalCommand {
|
||||
// and we create a ListStream that can be consumed
|
||||
|
||||
if redirect_stderr {
|
||||
let stderr = child.as_mut().stderr.take().ok_or_else(|| {
|
||||
let stderr = child.stderr.take().ok_or_else(|| {
|
||||
ShellError::ExternalCommand(
|
||||
"Error taking stderr from external".to_string(),
|
||||
"Redirects need access to stderr of an external command"
|
||||
@ -277,7 +275,7 @@ impl ExternalCommand {
|
||||
}
|
||||
|
||||
if redirect_stdout {
|
||||
let stdout = child.as_mut().stdout.take().ok_or_else(|| {
|
||||
let stdout = child.stdout.take().ok_or_else(|| {
|
||||
ShellError::ExternalCommand(
|
||||
"Error taking stdout from external".to_string(),
|
||||
"Redirects need access to stdout of an external command"
|
||||
@ -315,7 +313,7 @@ impl ExternalCommand {
|
||||
}
|
||||
}
|
||||
|
||||
match child.as_mut().wait() {
|
||||
match child.wait() {
|
||||
Err(err) => Err(ShellError::ExternalCommand(
|
||||
"External command exited with error".into(),
|
||||
err.to_string(),
|
||||
|
@ -1,111 +0,0 @@
|
||||
use std::process::{Child, Command};
|
||||
|
||||
/// A simple wrapper for `std::process::Command`
|
||||
///
|
||||
/// ## spawn behavior
|
||||
/// ### Unix
|
||||
/// When invoke `spawn`, current process will ignore `SIGTTOU`, `SIGTTIN` signal, spawned child process will get it's
|
||||
/// own process group id, and it's going to foreground(by making stdin belong's to child's process group).
|
||||
///
|
||||
/// When child is to over, `SIGTTOU`, `SIGTTIN` signal will be reset, foreground process is back to callers' process.
|
||||
///
|
||||
/// ### Windows
|
||||
/// It does nothing special on windows system, `spawn` is the same as [std::process::Command::spawn](std::process::Command::spawn)
|
||||
pub struct ForegroundProcess {
|
||||
inner: Command,
|
||||
}
|
||||
|
||||
/// A simple wrapper for `std::process::Child`
|
||||
///
|
||||
/// It can only be created by `ForegroundProcess::spawn`.
|
||||
pub struct ForegroundChild {
|
||||
inner: Child,
|
||||
}
|
||||
|
||||
impl ForegroundProcess {
|
||||
pub fn new(cmd: Command) -> Self {
|
||||
Self { inner: cmd }
|
||||
}
|
||||
|
||||
pub fn spawn(&mut self) -> std::io::Result<ForegroundChild> {
|
||||
fg_process_setup::prepare_to_foreground(&mut self.inner);
|
||||
self.inner.spawn().map(|child| {
|
||||
fg_process_setup::set_foreground(&child);
|
||||
ForegroundChild { inner: child }
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
impl AsMut<Child> for ForegroundChild {
|
||||
fn as_mut(&mut self) -> &mut Child {
|
||||
&mut self.inner
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for ForegroundChild {
|
||||
fn drop(&mut self) {
|
||||
// It's ok to use here because we have called `set_foreground` during creation.
|
||||
unsafe { fg_process_setup::reset_foreground_id() }
|
||||
}
|
||||
}
|
||||
|
||||
// It's a simpler version of fish shell's external process handling.
|
||||
//
|
||||
// For more information, please check `child_setup_process` function in fish shell.
|
||||
// https://github.com/fish-shell/fish-shell/blob/3f90efca38079922b4b21707001d7bb9630107eb/src/postfork.cpp#L140
|
||||
#[cfg(target_family = "unix")]
|
||||
mod fg_process_setup {
|
||||
use std::os::unix::prelude::CommandExt;
|
||||
pub(super) fn prepare_to_foreground(external_command: &mut std::process::Command) {
|
||||
unsafe {
|
||||
libc::signal(libc::SIGTTOU, libc::SIG_IGN);
|
||||
libc::signal(libc::SIGTTIN, libc::SIG_IGN);
|
||||
|
||||
// Safety:
|
||||
// POSIX only allows async-signal-safe functions to be called.
|
||||
// And `setpgid` is async-signal-safe function according to:
|
||||
// https://manpages.ubuntu.com/manpages/bionic/man7/signal-safety.7.html
|
||||
// So we're ok to invoke `libc::setpgid` inside `pre_exec`.
|
||||
external_command.pre_exec(|| {
|
||||
// make the command startup with new process group.
|
||||
// The process group id must be the same as external commands' pid.
|
||||
// Or else we'll failed to set it as foreground process.
|
||||
// For more information, check `fork_child_for_process` function:
|
||||
// https://github.com/fish-shell/fish-shell/blob/023042098396aa450d2c4ea1eb9341312de23126/src/exec.cpp#L398
|
||||
libc::setpgid(0, 0);
|
||||
Ok(())
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// If `prepare_to_foreground` function is not called, the function will fail with silence and do nothing.
|
||||
pub(super) fn set_foreground(process: &std::process::Child) {
|
||||
// it's ok to use unsafe here
|
||||
// the implementaion here is just the same as
|
||||
// https://docs.rs/nix/latest/nix/unistd/fn.tcsetpgrp.html, which is a safe function.
|
||||
unsafe {
|
||||
libc::tcsetpgrp(libc::STDIN_FILENO, process.id() as i32);
|
||||
}
|
||||
}
|
||||
|
||||
/// Reset foreground to current process, and reset back `SIGTTOU`, `SIGTTIN` single handler.
|
||||
///
|
||||
/// ## Safety
|
||||
/// It can only be called when you have called `set_foreground`, or results in undefined behavior.
|
||||
pub(super) unsafe fn reset_foreground_id() {
|
||||
libc::tcsetpgrp(libc::STDIN_FILENO, libc::getpgrp());
|
||||
libc::signal(libc::SIGTTOU, libc::SIG_DFL);
|
||||
libc::signal(libc::SIGTTIN, libc::SIG_DFL);
|
||||
}
|
||||
}
|
||||
|
||||
// TODO: investigate if we can set foreground process through windows system call.
|
||||
#[cfg(target_family = "windows")]
|
||||
mod fg_process_setup {
|
||||
|
||||
pub(super) fn prepare_to_foreground(_external_command: &mut std::process::Command) {}
|
||||
|
||||
pub(super) fn set_foreground(_process: &std::process::Child) {}
|
||||
|
||||
pub(super) unsafe fn reset_foreground_id() {}
|
||||
}
|
@ -1,4 +1,3 @@
|
||||
mod foreground;
|
||||
#[cfg(any(target_os = "android", target_os = "linux"))]
|
||||
mod linux;
|
||||
#[cfg(target_os = "macos")]
|
||||
@ -6,7 +5,6 @@ mod macos;
|
||||
#[cfg(target_os = "windows")]
|
||||
mod windows;
|
||||
|
||||
pub use self::foreground::{ForegroundChild, ForegroundProcess};
|
||||
#[cfg(any(target_os = "android", target_os = "linux"))]
|
||||
pub use self::linux::*;
|
||||
#[cfg(target_os = "macos")]
|
||||
|
Loading…
Reference in New Issue
Block a user