Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std::process::Command hangs if piped stdout buffer fills #45572

Open
Fraser999 opened this issue Oct 27, 2017 · 7 comments
Open

std::process::Command hangs if piped stdout buffer fills #45572

Fraser999 opened this issue Oct 27, 2017 · 7 comments
Labels
A-process Area: `std::process` and `std::env` C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Fraser999
Copy link

There's an issue on Windows when using Stdio::piped() for stdout. If the child process writes too many bytes to stdout, it appears to get permanently blocked. A minimal test case (echo 2049 blank lines to cmd) is:

fn main() {
    ::std::process::Command::new("cmd")
        .args(&["/c", "for /l %i in (1, 1, 2049) do @echo["])
        .stdout(::std::process::Stdio::piped())
        .status();
}

This hangs for me, but succeeds if only 2048 echos are specified (which is why I'm suspecting it's related to filling the 4096-byte buffer).

@retep998
Copy link
Member

So the issue is that .status() does not read from piped stdout/stderr, causing it to get blocked?

@Fraser999
Copy link
Author

I'm guessing that's the case, yes. Or it's trying to read and failing, but I haven't looked into the code there, so not really sure if that's even possible.

@pietroalbini pietroalbini added O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Jan 23, 2018
@oconnor663
Copy link
Contributor

I wonder if the simple fix is to have status silently convert piped to null for stdout and stderr.

@Heliozoa
Copy link
Contributor

Heliozoa commented Feb 18, 2021

I believe I've hit the same issue on Ubuntu 20.04.2.
Given

# Cargo.toml

[package]
name = "sandbox-rs"
version = "0.1.0"
authors = ["Heliozoa"]
edition = "2018"
// src/bin/printer.rs

fn main() {
    for _ in 0..65536 {
        print!("a");
    }
}
// src/main.rs

fn main() {
    let mut command = std::process::Command::new("./target/debug/printer");
    let _res = command
        .stdout(std::process::Stdio::piped())
        .status()
        .unwrap();
}

the process hangs when running cargo build --bin printer && time cargo run --bin sandbox-rs. Changing the status call to output or reducing the amount of iterations in printer.rs to 65535 fixes the problem. It's pretty niche and I only came across this when debugging another problem and trying different things out, but for what it's worth I expected this to work without issues, mainly because everything seems perfectly fine until you hit the limit.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Mar 14, 2021
Demonstrate best practice for feeding stdin of a child processes

Documentation change.

It's possible to create a deadlock with stdin/stdout I/O on a single thread:

* the child process may fill its stdout buffer, and have to wait for the parent process to read it,
* but the parent process may be waiting until its stdin write finishes before reading the stdout.

Therefore, the parent process should use separate threads for writing and reading.

These examples are not deadlocking in practice, because they use short strings, but I think it's better to demonstrate code that works even for long writes. The problem is non-obvious and tricky to debug (it seems that even libstd has a similar issue: rust-lang#45572).

This also demonstrates how to use stdio with threads: it's not obvious that `.take()` can be used to avoid fighting with the borrow checker.

I've checked that the modified examples run fine.
@LuchoBazz
Copy link

LuchoBazz commented Jun 12, 2021

I have the same problem in Ubuntu 20.10

rustc --version: rustc 1.52.1 (9bc8c42 2021-05-09)
cargo --version: cargo 1.52.0 (69767412a 2021-04-21)

I have a random c++ generator that generates between 1e5 and 2e5 numbers in c++17

#include <bits/stdc++.h>
using namespace std;
template <typename T>
T random(const T from, const T to) {
    static random_device rdev;
    static default_random_engine re(rdev());

    using dist_type = typename conditional<
        is_floating_point<T>::value,
        uniform_real_distribution<T>,
        uniform_int_distribution<T>
    >::type;

    dist_type uni(from, to);
    return static_cast<T>(uni(re));
}
int main() {
    int n = random<int>(1e5, 2e5);
    cout << n << endl;
    for(int i=0;i<n;++i) cout << random<int>(1, 1e9) << " ";
    cout << endl;
    return 0;
}

and what I am doing is running it from rust using a pipe for the generator output data

// compile
Command::new("g++")
    .arg("-std=c++17")
    .arg("-o")
    .arg("app.o")
    .arg("main.cpp")
    .status()
    .expect("Compiling C++ error");

// timeout 10sec
let timeout = 10000;
let now: Instant = Instant::now();

// run generator
let child = Command::new("app.o")
         .stdout(Stdio::piped())
         .stderr(Stdio::piped())
         .spawn()
         .unwrap();

// output file
let stdout = Some(PathBuf::from("output.txt"));

let output_file: Option<Arc<Mutex<File>>> = match stdout {
	Some(file) => {
	    let output = File::create(stdout).unwrap();
	    Some(Arc::new(Mutex::new(output)))
	},
	_ => None,
};

// check timeout
let thread: std::thread::JoinHandle<()> = std::thread::spawn(move || {
	for _ in 0..timeout {
	    if let Ok(Some(_)) = child.try_wait() {
		if let Ok(response) = child.wait_with_output() {
		    match output_file {
		        Some(f) => {
                            // write the output data
		            let mut file: std::sync::MutexGuard<File> = f.lock().unwrap();
		            file.write_all(&response.stdout).unwrap();
		        },
		        None => (),
		    }
		}
		return;
	    }
	    std::thread::sleep(std::time::Duration::from_millis(1));
	}
	child.kill().unwrap();
});

thread.join().unwrap();
let new_now: Instant = Instant::now();
let time: Duration = new_now.duration_since(now);

println!("{:?}", time);

I am using .spawn (), but it is blocking and I think it is because it exceeds the capacity of the buffer and it goes out due to the timeout because when I generate data less than 1e3 it works perfectly

Is there any way to fix this problem?
Or it is possible to increase the buffer capacity?

@oconnor663
Copy link
Contributor

oconnor663 commented Jun 13, 2021

@LuisMBaezCo the buffer size you're referring to is what Linux calls the "pipe capacity". This is an OS thing, and not something that Rust or libstd tries to mess with. If you know you want to increase the pipe capacity, you can look into the fcntl(F_SETPIPE_SZ) approach described here and here. (The equivalent can probably be done in safe Rust using the fcntl function provided by the nix crate, and using the file descriptors that you get from ChildStdout::as_raw_fd and ChildStderr::as_raw_fd.)

However, unless you're sure that's the right approach for you, I think you should avoid relying on pipe capacity. I think the right approach here is almost always to make sure that your parent process continually clears space in the stdout and stderr pipes by reading from them. (This requires at least one helper thread if you're setting both stdout and stderr to piped, unless you want to reach for fancy async IO.) For example, spawning helper threads to continually clear space is the strategy that the duct crate uses. If you're worried that there might be too much output for the parent to fit everything in memory, and the parent can't make use of the output in some sort of streaming fashion, then the next best alternative I'd suggest is to redirect the child's stdout and stderr to files.

umegaya added a commit to suntomi/deplo that referenced this issue Dec 28, 2021
…s::Command does not work well with huge (>1kb) output piping. see rust-lang/rust#45572 for detail
umegaya added a commit to suntomi/deplo that referenced this issue Dec 28, 2021
…s::Command does not work well with huge (>1kb) output piping. see rust-lang/rust#45572 for detail
@ChrisDenton
Copy link
Member

ChrisDenton commented Apr 4, 2022

The Windows specific part of this issue can be solved by using a larger buffer size. We currently set a relatively small buffer size, at least compared to typical Linux defaults.

The more general issue is harder to solve within the std but maybe it can be mitigated somewhat by improving documentation and providing workarounds where we can. So my suggestions for addressing this issue would be:

  1. Increases the pipe buffer size on Windows (see link above). Implemented in Windows: Increase a pipe's buffer capacity to 64kb #95782
  2. Document the potential deadlock if the child's stdout buffer is filled without being read and note how the user can avoid this situation.
  3. For the specific case of Stdio::piped() being used for the child's stdout and then Command::status being called, I think it could be automatically replaced with Stdio::null() without changing any user visible behaviour (except avoiding deadlocks). Though I'm not totally sure about this.

@rustbot label +E-help-wanted

@rustbot rustbot added the E-help-wanted Call for participation: Help is requested to fix this issue. label Apr 4, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 8, 2022
Windows: Increase a pipe's buffer capacity to 64kb

This brings it inline with typical Linux defaults: https://www.man7.org/linux/man-pages/man7/pipe.7.html

> Since Linux 2.6.11, the pipe capacity is 16 pages (i.e., 65,536 bytes in a system with a page size of 4096 bytes).

This may also help with rust-lang#45572 and rust-lang#95759 but does not fix either issue. It simply makes them much less likely to be encountered.
@ChrisDenton ChrisDenton removed the O-windows Operating system: Windows label Apr 8, 2022
@ChrisDenton ChrisDenton changed the title std::process::Command hangs on Windows if piped stdout buffer fills std::process::Command hangs if piped stdout buffer fills Apr 8, 2022
BrynCooke pushed a commit to apollographql/router that referenced this issue Apr 19, 2022
OSX had an issue with max UDP packet size
Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572
BrynCooke pushed a commit to apollographql/router that referenced this issue Apr 19, 2022
OSX had an issue with max UDP packet size
Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572
BrynCooke pushed a commit to apollographql/router that referenced this issue Apr 21, 2022
OSX had an issue with max UDP packet size
Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572
BrynCooke pushed a commit to apollographql/router that referenced this issue Apr 22, 2022
OSX had an issue with max UDP packet size
Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572
BrynCooke added a commit to apollographql/router that referenced this issue Apr 22, 2022
* Move the apollo config, update changelog

* Split configuration for Apollo so that tracing can go in the tracing module and metrics can go in the metrics module.

* Introduce instrument layer
Move router_request span to telemetry plugin. There's no way to disable this plugin so it's OK.
Add logic for dynamic client name and client version header.

* Print errors if config could not be reloaded.

* Update test config files

* Integration test for jaeger

* Fix build for windows and osx.
OSX had an issue with max UDP packet size
Windows has all sorts of file related issues. On the plus side the special handling for windows in circle has been removed as the root cause of the hang on windows is now known to be: rust-lang/rust#45572

* Add logic to kill node processes on test termination

* Remove all piping from external commands, it's not safe unless we read via another thread (which we don't)

* Removed instructions for developing without docker compose. It is only going to get more complicated aw we add more integration tests. Docker compose is the way to go.

* Router span name moved to constant.


Co-authored-by: bryn <[email protected]>
Co-authored-by: Gary Pennington <[email protected]>
@workingjubilee workingjubilee added the A-process Area: `std::process` and `std::env` label Jul 22, 2023
@Enselic Enselic added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-process Area: `std::process` and `std::env` C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants