clap-rs / clap

A full featured, fast Command Line Argument Parser for Rust
docs.rs/clap
Apache License 2.0
13.64k stars 1.02k forks source link

Exit without flushing stdout and stderr #5542

Closed casey closed 1 week ago

casey commented 1 week ago

I noticed the calls to flush standard out and err before calling std::process:exit() and I believe they aren't necessary.

Standard out is flushed when calling std::process::exit():

And standard error is unbuffered, and so doesn't need to be flushed.

I noticed this function was added in PR #1873, to fix issue #1871.

I checked out 4675070a, the commit before #1873 was merged, and wasn't able to reproduce the issue in #1871, whose repro steps are to run cargo run --example 09_auto_version -- --version and not see any version output.

So I'm thinking it's likely that at some point, the rust stdlib changed to flush stdout before exiting.

Since clap has MSRV 1.74, I installed 1.74.0 and ran cargo +1.74.0 run --example 09_auto_version -- --version to make sure that things still worked with the oldest supported rust version.

epage commented 1 week ago

For my own curiosity, how did you notice this and how does the current behavior impact users?

casey commented 1 week ago

Good question! I noticed this because I had a function fn run(args: &[&str]) -> Result<(), i32> which parsed args with clap, did stuff, and then returned. The calling function was fn main(), and would just get the i32 exit code on an error and call std::process::exit() with it. However, I was using get_matches_from(), which meant that if there was an error parsing args, the program would exit. I wanted to return an error code in all cases, so I switched to try_get_matches_from(), and looked at the implementation of get_matches_from() to see how to replicate its behavior. Digging in, I found the calls to flush stderr and stdout, so I thought that I would have to flush stderr and stdout in main in order to ensure that any buffered data was flushed. However, digging around, it seemed like it wasn't actually necessary. So in my case, the benefit is more not suggesting to users that if they call try_matches_from() they need to worry about flushing the standard streams. It eliminates a couple of system calls, but since that only happens once before terminating the process, any performance benefit is probably negligable.

casey commented 1 week ago

And the reason I wanted run() to not exit the process was so it could used in contexts other than main where it wouldn't be desirable to exit the process.

epage commented 1 week ago

Wow, you really dug deep for that question. Finally got some time to look at the background and the change. Seems reasonable.