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

Handle terminal signals #128

Merged
merged 10 commits into from
Nov 22, 2017
30 changes: 30 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ lazy_static = "0.2.9"
num_cpus = "1.6.2"
regex = "0.2"
regex-syntax = "0.4"
ctrlc = "3.0"

[target.'cfg(all(unix, not(target_os = "redox")))'.dependencies]
libc = "0.2"
Expand Down
18 changes: 15 additions & 3 deletions src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,18 @@ use std::{fs, process};
use std::io::{self, Write};
use std::ops::Deref;
use std::path::{self, Path, PathBuf, Component};
use std::sync::Arc;
use std::sync::atomic::{AtomicBool, Ordering};
#[cfg(any(unix, target_os = "redox"))]
use std::os::unix::fs::PermissionsExt;

use ansi_term;

pub fn print_entry(entry: &PathBuf, config: &FdOptions) {
pub fn print_entry(entry: &PathBuf, config: &FdOptions, wants_to_quit: &Arc<AtomicBool>) {
let path = entry.strip_prefix(".").unwrap_or(entry);

let r = if let Some(ref ls_colors) = config.ls_colors {
print_entry_colorized(path, config, ls_colors)
print_entry_colorized(path, config, ls_colors, &wants_to_quit)
} else {
print_entry_uncolorized(path, config)
};
Expand All @@ -33,7 +35,12 @@ pub fn print_entry(entry: &PathBuf, config: &FdOptions) {
}
}

fn print_entry_colorized(path: &Path, config: &FdOptions, ls_colors: &LsColors) -> io::Result<()> {
fn print_entry_colorized(
path: &Path,
config: &FdOptions,
ls_colors: &LsColors,
wants_to_quit: &Arc<AtomicBool>,
) -> io::Result<()> {
let default_style = ansi_term::Style::default();

let stdout = io::stdout();
Expand Down Expand Up @@ -63,6 +70,11 @@ fn print_entry_colorized(path: &Path, config: &FdOptions, ls_colors: &LsColors)
// Everything else uses a separator that is painted the same way as the component.
_ => style.paint(path::MAIN_SEPARATOR.to_string()).to_string(),
};

if wants_to_quit.load(Ordering::Relaxed) {
write!(handle, "\n")?;
process::exit(0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to complain, since I likely won't have the time to contribute a fix myself, but I just noticed this while looking through the change log. According to the docs, it's generally not good practice to just process::exit in the middle of the program's execution.

Note that because this function never returns, and that it terminates the process, no destructors on the current stack or any other thread's stack will be run. If a clean shutdown is needed it is recommended to only call this function at a known point where there are no more destructors left to run.

That is, it should really be run at the very end of the main function. So rather than call process::exit here, it should return an error value of some kind that gets propagated all the way up to the main function.

Also, if you're going to handle signals, it's probably better to handle them in the general case, rather than just when there are colors. Messing up the terminal colors is obviously the more pressing case because it messes up the user's whole shell, but the program as a whole should handle exiting gracefully.

Also, this should probably not exit with code 0, which generally means success. Ctrl-C is usually an error condition for a program.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead10ck thanks for bringing this to our attention.

I was not aware of this behavior from process::exit, a quick fix to remove the process:exit(0) would be to replace it with something like return Err(Error::new(ErrorKind::Interrupted, "Interrupted"));. I'm just not sure if we need to do something similar in the other spawned thread though. What happens if a rust program exits with outstanding threads executing?

We could handle the terminal signal in the general case, but we handle SIGINT for tear down purposes. We don't have to tear down in the non-colorized case so there's no point in handling SIGINT at that point. We can just use the default SIGINT handling to exit when output is not colorized.

SIGINT is generally for graceful shutdown of a program, I would not say it is generally an error condition. The application handles SIGINT and exits in a known and expected fashion, this would be a successful execution of the program in my opinion.

Copy link

@dead10ck dead10ck Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware of this behavior from process::exit, a quick fix to remove the process:exit(0) would be to replace it with something like return Err(Error::new(ErrorKind::Interrupted, "Interrupted"));. I'm just not sure if we need to do something similar in the other spawned thread though. What happens if a rust program exits with outstanding threads executing?

I'm sorry, I don't actually have much experience with this in Rust; this is just a tidbit of information I happened to recall when glancing over the changes.

We could handle the terminal signal in the general case, but we handle SIGINT for tear down purposes. We don't have to tear down in the non-colorized case so there's no point in handling SIGINT at that point. We can just use the default SIGINT handling to exit when output is not colorized.

I haven't done my research here, but if I'm not mistaken, the default behavior is not graceful—it essentially just aborts the program in the middle, just like calling process::exit. In any case, though, I think if you're going to go to the trouble of handling graceful exiting, why not handle it everywhere, if it's not too much extra effort?

SIGINT is generally for graceful shutdown of a program, I would not say it is generally an error condition. The application handles SIGINT and exits in a known and expected fashion, this would be a successful execution of the program in my opinion.

I can understand your perspective, but this is not conventional. POSIX specifies that a process terminated by a signal should have an exit code greater than 128. You can test this for yourself with any other standard Linux utility, like find, grep, etc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover, it occurred to me that if you only handle the signal when there are colors, and exit with 0 in that case, fd will exit with 0 when there are colors and non-0 when there aren't:

~ fd '' / --color always
...
/proc/9/coredump_filter
^C
/
➜  ~ echo $?               
0
➜  ~ fd '' / --color never
...
/var/log/cups/page_log.3
/var/log/puppetlabs/puppet
/var/log/cups/access_log.4
/var/log/cups/access_log.2
/var/log/cups/page_log.1
/var/log/cu^C
➜  ~ echo $?
130

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead10ck, looks like you're right about the exit code.

It should also be noted that while process::exit does not call destructors on exit the memory is not going to really be leaked on any standard unix or windows operating system. The OS keeps track of all memory allocated by a program and will free that memory when the program exits. Only on specialized operating systems such as real-time operating systems do you run the chance of the OS not cleaning up memory after program exit.

For the time being I think we can just replace the exit code with 130 so fd reports it was killed by SIGINT correctly.

On the topic of general SIGINT handling, we have only one special case that requires delaying the exit until after the thread is done writing to the terminal. There's no need in delaying exit when terminal output is not colorized, hence custom shutdown only in the colorized case.

@sharkdp do you have any preferences on these matters?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dead10ck Thank you for pointing this out

@Doxterpepper In this case, exiting with 130 seems like a reasonable first step. However, we should maybe open a new ticket in order to refactor the code such that error handling like this would be easier.

}
}

if config.null_separator {
Expand Down
20 changes: 17 additions & 3 deletions src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@
// notice may not be copied, modified, or distributed except
// according to those terms.

extern crate ctrlc;

use exec::{self, TokenizedCommand};
use fshelper;
use internal::{error, FdOptions};
use output;

use std::path::Path;
use std::sync::{Arc, Mutex};
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::mpsc::channel;
use std::thread;
use std::time;
Expand Down Expand Up @@ -60,6 +63,16 @@ pub fn scan(root: &Path, pattern: Arc<Regex>, config: Arc<FdOptions>) {
.threads(threads)
.build_parallel();

let wants_to_quit = Arc::new(AtomicBool::new(false));

match config.ls_colors {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use if-let here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Some(_) => {
let wq = Arc::clone(&wants_to_quit);
ctrlc::set_handler(move || { wq.store(true, Ordering::Relaxed); }).unwrap();
}
None => (),
}

// Spawn the thread that receives all results through the channel.
let rx_config = Arc::clone(&config);
let receiver_thread = thread::spawn(move || {
Expand Down Expand Up @@ -113,7 +126,8 @@ pub fn scan(root: &Path, pattern: Arc<Regex>, config: Arc<FdOptions>) {
if time::Instant::now() - start > max_buffer_time {
// Flush the buffer
for v in &buffer {
output::print_entry(v, &rx_config);
output::print_entry(&v, &rx_config, &wants_to_quit);
output::print_entry(v, &rx_config, &wants_to_quit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this line duplicated?

}
buffer.clear();

Expand All @@ -122,7 +136,7 @@ pub fn scan(root: &Path, pattern: Arc<Regex>, config: Arc<FdOptions>) {
}
}
ReceiverMode::Streaming => {
output::print_entry(&value, &rx_config);
output::print_entry(&value, &rx_config, &wants_to_quit);
}
}
}
Expand All @@ -132,7 +146,7 @@ pub fn scan(root: &Path, pattern: Arc<Regex>, config: Arc<FdOptions>) {
if !buffer.is_empty() {
buffer.sort();
for value in buffer {
output::print_entry(&value, &rx_config);
output::print_entry(&value, &rx_config, &wants_to_quit);
}
}
}
Expand Down