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

Exit without flushing stdout and stderr #5542

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

casey
Copy link
Contributor

@casey casey commented Jun 20, 2024

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.

Standard out is flushed during `std::rt::cleanup()`, called by
`std::process::exit()`, and standard error is unbuffered, so doesn't
need to be flushed.
@casey casey force-pushed the exit-without-flushing branch from bc913b0 to 90b2661 Compare June 20, 2024 04:19
@epage
Copy link
Member

epage commented Jun 20, 2024

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

@casey
Copy link
Contributor Author

casey commented Jun 20, 2024

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
Copy link
Contributor Author

casey commented Jun 20, 2024

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
Copy link
Member

epage commented Jun 21, 2024

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

@epage epage merged commit cf151fd into clap-rs:master Jun 21, 2024
22 checks passed
@casey casey deleted the exit-without-flushing branch June 21, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants