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

Check for SIGHUP #78

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Check for SIGHUP #78

merged 1 commit into from
Jun 10, 2021

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented May 24, 2021

If you have the termination feature enabled and close the terminal (its window) with the X button, SIGHUP is sent to the process but ctrlc doesn't handle it so no graceful shutdown is done in that case but I would expect the termination feature to be able to detect that because pressing the X button to terminate an application is definitely a common kind of termination. Also, I believe on Windows this already works because when the window is X-ed, CTRL_CLOSE_EVENT is sent which ctrlc seems to handle. This means on Windows, the handler is invoked on X but on Linux (and very likely macOS), it is not. So this fixes inconsistent behavior.

match signal::sigaction(signal::Signal::SIGHUP, &new_action) {
Ok(_) => {}
Err(e) => {
signal::sigaction(signal::Signal::SIGINT, &_old).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Old value for SIGTERM should be reverted in this cleanup function. Also, the variable _old should be named old now as it is being used. Maybe assign current _old to sigint_old and introduce sigterm_old.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a change. Is that maybe what you have in mind?

@wooster0
Copy link
Contributor Author

wooster0 commented May 28, 2021

I took another look at https://en.wikipedia.org/wiki/Signal_(IPC)#POSIX_signals and found this other signal:

SIGTSTP
The SIGTSTP signal is sent to a process by its controlling terminal to request it to stop (terminal stop). It is commonly initiated by the user pressing Ctrl+Z. Unlike SIGSTOP, the process can register a signal handler for, or ignore, the signal.

This should probably be handled as well to ensure a graceful termination whenever possible.

Do you see anything else that might be worth checking for on the article?

Maybe SIGABRT and SIGQUIT?

src/platform/unix/mod.rs Outdated Show resolved Hide resolved
src/platform/unix/mod.rs Show resolved Hide resolved
src/platform/unix/mod.rs Outdated Show resolved Hide resolved
src/platform/unix/mod.rs Outdated Show resolved Hide resolved
@Detegr
Copy link
Owner

Detegr commented May 28, 2021

I took another look at https://en.wikipedia.org/wiki/Signal_(IPC)#POSIX_signals and found this other signal:

SIGTSTP
The SIGTSTP signal is sent to a process by its controlling terminal to request it to stop (terminal stop). It is commonly initiated by the user pressing Ctrl+Z. Unlike SIGSTOP, the process can register a signal handler for, or ignore, the signal.

This should probably be handled as well to ensure a graceful termination whenever possible.

Do you see anything else that might be worth checking for on the article?

Maybe SIGABRT and SIGQUIT?

I don't think SIGSTOP should be handled. It is not a termination, but just a temporary stop for a process that is going to be continued later. SIGABRT and SIGQUIT are program error signals, and I don't think it's a good idea to add a handler for those in any case.

@wooster0
Copy link
Contributor Author

@Detegr I made some changes and tested it. Do you mind taking a look again?

Copy link
Owner

@Detegr Detegr left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you rebase the branch to only have a single commit that does the change?

@wooster0
Copy link
Contributor Author

wooster0 commented Jun 8, 2021

Done.

@Detegr Detegr merged commit aad1cbf into Detegr:master Jun 10, 2021
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