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 #87

Closed
esyphelon opened this issue Oct 9, 2017 · 11 comments
Closed

Handle terminal signals #87

esyphelon opened this issue Oct 9, 2017 · 11 comments

Comments

@esyphelon
Copy link

Hey love the project, but I noticed the first time I used it(did a too wide search that I wanted to stop), that it doesn't seem to listen for signal interrupts very well(Sigterm and the like).

@sharkdp
Copy link
Owner

sharkdp commented Oct 9, 2017

Thank you for the feedback.

Is there anything specific that happened when you cancelled the process?

I'm not sure what the expected behavior would be. Sending SIGTERM or KILL seems to work fine for me.

@sharkdp
Copy link
Owner

sharkdp commented Oct 10, 2017

One thing I have noticed is that sometimes the last result will not be printed out completely:

/proc/263/oom_score_adj
/proc/256/oom_score_adj
/proc/322/smaps
/proc/263�[36^C

I'm not sure if this is something that should be taken care of, to be honest.

@Detegr I found your Ctrl-C handler while searching for signal handlers in Rust. Would this be something that could help here?

@esyphelon
Copy link
Author

esyphelon commented Oct 11, 2017

So what happened for me was I killed it(Ctrl-C) while the output was scrolling. It stopped scrolling and the console became non-responsive. I am away from the computer in question, I will see if I can reproduce tomorrow morning.

@Detegr
Copy link
Contributor

Detegr commented Oct 11, 2017

@Detegr I found your Ctrl-C handler while searching for signal handlers in Rust. Would this be something that could help here?

Sure, however you would need to fetch a value of an atomic variable after printing each line. My initial thought for a signal handler would be to print a reset sequence and exit the program to ensure that the terminal configuration is sane after terminating. However, fd locks stdout for printing so I'm not sure if this would be possible.

@esyphelon if you can reproduce the issue, try writing reset + enter to the terminal after terminating fd (even if it shows nothing) and see whether that fixes the issue.

@mmstick
Copy link
Contributor

mmstick commented Oct 15, 2017

I could implement this, if it's needed.

@sharkdp
Copy link
Owner

sharkdp commented Oct 15, 2017

It doesn't have the highest priority for me, but definitely "nice to have" 👍.

I'm a little bit afraid that it might complicate the code and add even more OS-specific features, but maybe that's just me.

Edit: it looks like Ctrl-C abstracts over OS-specifics.

@Doxterpepper
Copy link
Contributor

Has anyone been able to reproduce this reliably? I figure sending a SIGTERM while output is being displayed should reproduce it so I tried writing a script to reproduce the error but so far no luck. Maybe someone else can get it to work for them.

@sharkdp
Copy link
Owner

sharkdp commented Oct 16, 2017

@Doxterpepper Nice, thank you for looking into this. Have you tried running this with --color always? Otherwise, fd will not print any ANSI codes at all.

@Doxterpepper
Copy link
Contributor

oh, I know what's causing this precisely now. My script won't help, it's when you SIGTERM in the middle of printing ANSI escape codes used for the colorized output. If fd exits before a readable letter is written to the terminal then only the binary data is written and either gibberish is displayed or it will turn your prompt that color. Some terminals handle binary data poorly and that's why your terminal locks up, @esyphelon

@Doxterpepper
Copy link
Contributor

@mmstick , I don't want to step on any toes or anything but I have a simple fix for this if you don't mind me submitting a PR.

@sharkdp
Copy link
Owner

sharkdp commented Oct 17, 2017

I don't want to step on any toes or anything but I have a simple fix for this if you don't mind me submitting a PR.

Sounds good 😄

This was referenced Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants