-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
+59
−6
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
d2f5b33
Really messed up that last merge conflict. Fixing it here.
Doxterpepper 921a3a6
Fixing formatting with rustfmt
Doxterpepper 6ba4eff
Merge branch 'master' into fix-ctrlc
Doxterpepper baab135
Merge branch 'master' into fix-ctrlc
Doxterpepper 712f564
Merge branch 'master' of github.com:sharkdp/fd into fix-ctrlc
Doxterpepper d6e63f5
Merge branch 'fix-ctrlc' of github.com:Doxterpepper/fd into fix-ctrlc
Doxterpepper cfa7948
Changed match statement to if let
Doxterpepper f31063e
Merge branch 'master' into fix-ctrlc
Doxterpepper a43aca8
Merge branch 'master' of github.com:sharkdp/fd into fix-ctrlc
af809f2
Merge branch 'fix-ctrlc' of github.com:Doxterpepper/fd into fix-ctrlc
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.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.
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 theprocess:exit(0)
would be to replace it with something likereturn 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 handlesSIGINT
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?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.
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:There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.