Handle terminal signals #128
Conversation
|
Welcome and thank you very much! Do you have a method to reproduce the original error (such that we can prove that it is now solved)? |
|
Yea I'm going to look into a good test for this, right now I'm just killing fd in the middle of output to try and trigger the scenario. The problem is quitting coloured output before the reset sequence is printed, so the following script should never have garbage printed to the screen or turn the prompt a different colour.
|
Doxterpepper
force-pushed
the
fix-ctrlc
branch
from
11bae9f
to
921a3a6
Compare
|
For this kind of thing, I don't think we actually need an integration test (because this behavior is non-deterministic). I'm okay if there is just a simple command to try it out (like the one you provided). Thanks! (actually, I didn't have any "luck" in reproducing this, yet 😄) |
|
You can test with this command: |
|
Sorry for the late response. I still have trouble to reproduce the actual bug. What kind of terminal+shell combination do you use? If I understand correctly, you run |
|
I test with bash, without any method to reset the coloring state to normal (e.g. reset the color for every command prompt). Then you can try and see that the prompt has color blue after Ctrl-C. |
src/walk.rs
Outdated
| @@ -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 { | |||
src/walk.rs
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
Why is this line duplicated?
I see in theory how this would happen but in practice it does not happen to me. Tested in zsh, bash, and sh. |
|
So here's a screenshot of it on my system using the latest master branch of fd. I ran
|
|
@Doxterpepper Sorry that this has been dragging on for some time now. I'd like to merge this. I still cannot reproduce the messed-up prompt, but I sometimes do see some 'garbage' character on the screen - and I see that it's potentially causing problems in your screenshot. Could you please take @Detegr's comment into account and also rebase this on top of |
|
@sharkdp, it's no problem. This is definitely a hard one to reproduce, I've tried writing several scripts to detect it without success. I'll make that change recommended by Detger tonight. |
|
Has anyone tested performance implications of this change? Ctrlc crate spawns a new thread (for now, work is ongoing to get rid of it) and AFAIK spawning threads on Windows can be quite slow. However it's only one thread at startup so maybe I'm just being paranoid. |
|
@Detegr, I haven't really tested performance yet. I can look into that if you like but I don't have a windows machine to test on. |
|
Okay, I finally did some simple bench marking on a windows virtual machine. I used a windows 10 virtual machine with vmware fusion to run fd and used haskell bench command line tool to benchmark the different branches of fd. Each was compiled on the virtual machine. Test 1: searching the fd directoryfd '' .Master branchfix-ctrlc branchfd 'walk.rs' .Master branchfix-ctrlc branchTest 2: Searching C:\fd '' C:\Master branchfix-ctrlc branchfd 'walk.rs' C:\Master branchfix-ctrlc branch |
|
So I wasn't happy with those previous benchmarks, I gave three runs each because the timings were all over the place. Tonight I stopped all other applications on my laptop and set the confidence interval to 0.99 in bench which resulted in much more consistent results. I will only provide one output for each test now because the timings vary so little per test. If you would like to run these yourself, feel free, but on my machine running windows 10 in vmware fusion there is no performance hit for using the ctrl-c library even though it spins up a separate thread for a signal handler. Test 1: Searching the fd projectfd '' .Master branchfix-ctrlc branchfd 'walk.rs' .Master branchfix-ctrlc branchTest 2: Searching the whole file systemfd '' C:\Master branchfix-ctrlc branchfd 'walk.rs' C:\Master branchfix-ctrlc branch |
|
Looks great! 👍 |
|
@Doxterpepper Fantastic work! Thank you very much for these benchmarks (and for your patience). I have also tested the performance on Linux and also could not measure any significant difference. |
|
|
||
| if wants_to_quit.load(Ordering::Relaxed) { | ||
| write!(handle, "\n")?; | ||
| process::exit(0); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 $?
130There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
Adding signal handler for colorized output to prevent garbage from being printed to the screen.
#87