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

Sending a signal to all processes in the processgroup of the pts #167

Closed
Daniel-Abrecht opened this issue Dec 14, 2017 · 4 comments
Closed
Labels
enhancement help wanted Issues identified as good community contribution opportunities
Milestone

Comments

@Daniel-Abrecht
Copy link
Contributor

Daniel-Abrecht commented Dec 14, 2017

This is only relevant to the unix version. I initially thought I could just use the pty.kill function to simulate pressing Ctrl+C by sending a SIGINT, but it didn't work. After further investigation, I found out that the pty.kill function only sends a signal to a single process, instead of the whole process group.

Just for Testing purposes, I added a pty.signal function which uses the TIOCSIG ioctl to send a signal to all processes in the process group of the pts. It works the why I expected, but it's not ideal since it currently only accepts the signal number and not the signal name as argument:
https://github.com/Daniel-Abrecht/node-pty/commit/a1e1e99b6a83ae80db9d1f5f4b3f477a1c73b70f

I'm not sure if adding such a function is the right way to go, it may make more sense to replace the pty.kill function by this instead.

@Tyriar
Copy link
Member

Tyriar commented Dec 15, 2017

@jerch do you think .kill should move over to sending the signal to all processes rather than killing them?

@jerch
Copy link
Collaborator

jerch commented Dec 15, 2017

I assume most people who use pty.kill do this to close all slave processes to free the pty device afterwards. Normally this should be done via SIGHUP which kills the whole session on the slave side (all sub processes as long as they got not detached from the controlling pty terminal). Thats what pty.kill('SIGHUP') does at the moment, which is fine.
But this automatic mechanism is only true for SIGHUP. Most other signals dont "bubble" - here @Daniel-Abrecht has a valid point: those other signals only get delivered to the first slave process, which is IMHO a surprising behavior: SIGHUP affects all slave processes while other signals only alter the first process.
Whether to change this - yes I think so, for several reasons:

  • the kill method on the pty device object somewhat implies "signal control" over all slave side children processes which is only true for SIGHUP atm
  • the change will hardly break any existing code since people typically dont bother calling pty.kill with some other signal
  • last and maybe most crucial - calling pty.kill with some other signal might lead to broken or zombie processes with the current implementation

I suggest to go with the current implementation if no signal or SIGHUP was requested. For all other signals switch to the process group. Note that TIOCSIG is only defined on BSD like systems and Linux, under Solaris it is TIOCSIGNAL.

@Tyriar Tyriar added enhancement help wanted Issues identified as good community contribution opportunities labels Dec 15, 2017
@Daniel-Abrecht
Copy link
Contributor Author

I've created a pull request for these changes: #169

@Tyriar
Copy link
Member

Tyriar commented Dec 31, 2017

Fixed in #169

@Tyriar Tyriar closed this as completed Dec 31, 2017
@Tyriar Tyriar added this to the 0.7.5 milestone Dec 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted Issues identified as good community contribution opportunities
Projects
None yet
Development

No branches or pull requests

3 participants