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

Send SIGHUP to process group when SIGHUP is received #494

Merged
merged 1 commit into from Oct 15, 2017

Conversation

Projects
None yet
3 participants
@doppioandante
Contributor

doppioandante commented Oct 1, 2017

If the process is a controlling process, the SIGHUP signal shall be sent to each process in the foreground process group of the controlling terminal belonging to the calling process.

The solution works but I don't think this is the right one. I'll put it here in case there are comments. I will look at what other shell do tomorrow.

@doppioandante

This comment has been minimized.

Contributor

doppioandante commented Oct 12, 2017

Ok so, I've been delving into the dash shell source code and signal handling is a really big chunk of the code.
I will need more time to study it, but for now I think this PR can be merged (I've been using it since then on my system and everything works fine).

@krader1961

This comment has been minimized.

krader1961 commented Oct 13, 2017

I am too new to this project (and the Go language) to comment on the correctness of this change. But I've been using UNIX and have been paid to support it (including kernel crash dump analysis) for more than 30 years. Changes to signal handling and job control need careful consideration. The fish shell has had many problems related to the one being fixed by this PR -- some of which have been fixed in the past couple of years -- because the person who wrote the original code didn't understand how UNIX shells should handle these situations. So I'd feel better about seeing this merged if @doppioandante told me they had read the relevant sections of the "Advanced Programming in the UNIX Environment" (APUE) by W. R. Stevens and Stephen Rago. 😄 Also, as far as I can tell from skimming the code elvish doesn't manipulate process groups or the controlling tty. Which makes me nervous about seeing this merged.

@doppioandante

This comment has been minimized.

Contributor

doppioandante commented Oct 13, 2017

@krader1961 lol! Unfortunately that book is in my ginormous "books to read" pile, and not even at the top.
I can definitely live without this being merged (I will just keep using it myself).
The reason why I needed this is that some terminals just send SIGHUP to the underlying shell when quit, thus elvish won't terminate, nor programs running in it (and this caused pretty weird problems to me, like CPU spinning at 100%).
The fact that signal handling is underestimated in elvish is undeniable (at least in comparison to what I have seen in a POSIX shells like dash).

@krader1961

This comment has been minimized.

krader1961 commented Oct 14, 2017

FWIW, I did not mean to imply that this should not be merged. Only that based on past experience, especially with the fish-shell project, I think changes like this one need a slightly higher degree of confidence about their correctness than a typical feature change. I certainly don't want to see elvish, or its child processes, spinning if the enclosing shell (or ssh session) is terminated just because elvish doesn't handle SIGHUP correctly. I'll see if I can reproduce that unwanted behavior without this change then apply the change and see if the expected behavior is observed.

@doppioandante

This comment has been minimized.

Contributor

doppioandante commented Oct 14, 2017

@krader1961 I can reproduce this with my terminal emulator, Tilix.
On qterminal elvish terminated correctly.
An easy way to reproduce is by sending the SIGHUP signal to elvish and watch as it ignores it.

@xiaq

This comment has been minimized.

Member

xiaq commented Oct 15, 2017

This PR seems pretty harmless, I am merging it. Sending SIGHUP to the whole process group sounds like a reasonable thing to do for a non-job-control shell.

@xiaq xiaq merged commit f6c4671 into elves:master Oct 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment