lxsang / PTerm

MIT License
34 stars 8 forks source link

Processes are no longer sent SIGHUP when quitting Pharo with open Terminal windows #58

Closed Rinzwind closed 4 months ago

Rinzwind commented 11 months ago

When quitting Pharo with open Terminal windows, the processes no longer seem to be sent SIGHUP.

The following snippet loads a previous version of PTerm, then opens a Terminal window to run ‘siglog’, waits 15 seconds, and quits the image:

committish := '0974fb3f21a28d5263f307fd3974ace1c74af1a9'.
fileReference := FileLocator imageDirectory / 'siglog'.
arguments := { DateAndTime now asString }.

fileReference exists ifFalse: [ Error signal ].
Metacello new
    repository: ('github://lxsang/PTerm:{1}' format: { committish });
    baseline:'PTerm';
    load.
(Smalltalk at: #TerminalEmulator) open: fileReference fullName arguments: arguments.
[
    (Delay forSeconds: 15) wait.
    Smalltalk snapshot: false andQuit: true.
] fork

The log of ‘siglog’ shows ‘Received signal: 1’ as expected.

But when doing the snippet again after changing ‘committish’ to 'master', the log of ‘siglog’ does not show ‘Received signal: 1’.

The source for ‘siglog’ and the command for showing its log can be found in a comment in pull request #23.

lxsang commented 11 months ago

Yeah, we got this problem due to the use of posix_spawnp to spawn the shell:

The current Linux-specific implementation of posix_spawn() in glibc blocks all signals on the parent process.

From glibc's spawni.c

The Linux implementation of posix_spawn{p} uses the clone syscall directly
   with CLONE_VM and CLONE_VFORK flags and an allocated stack.  The new stack
   and start function solves most the vfork limitation (possible parent
   clobber due stack spilling). The remaining issue are:

   1. That no signal handlers must run in child context, to avoid corrupting
      parent's state.
   2. The parent must ensure child's stack freeing.
   3. Child must synchronize with parent to enforce 2. and to possible
      return execv issues.

   The first issue is solved by blocking all signals in child, even
   the NPTL-internal ones (SIGCANCEL and SIGSETXID).  The second and
   third issue is done by a stack allocation in parent, and by using a
   field in struct spawn_args where the child can write an error
   code. CLONE_VFORK ensures that the parent does not run until the
   child has either exec'ed successfully or exited.
Rinzwind commented 10 months ago

I’m not sure that applies in my case as I’m using macOS.

Below is another snippet that loads a previous version of PTerm. Instead of using #snapshot:andQuit: to quit, it sends SIGKILL to the Pharo process. The ‘siglog’ process is still sent SIGHUP. If I change the assignment to ‘removeTIOCSCTTY’ to assign ‘true’, then the ‘siglog’ process is no longer sent SIGHUP.

In the ‘master’ version, ‘pterm_spawn_tty’ has been removed, its call of ‘setsid’ was replicated by the use of ‘POSIX_SPAWN_SETSID’ in #spawnAttrSetting but I’m not sure the call of ‘ioctl’ with ‘TIOCSCTTY’ was replicated.

committish := '0974fb3f21a28d5263f307fd3974ace1c74af1a9'.
removeTIOCSCTTY := false.
fileReference := FileLocator imageDirectory / 'siglog'.
arguments := { DateAndTime now asString }.

fileReference exists ifFalse: [ Error signal ].
Metacello new
    repository: ('github://lxsang/PTerm:{1}' format: { committish });
    baseline: 'PTerm';
    load.
removeTIOCSCTTY ifTrue: [
    Author useAuthor: 'Anonymous' during: [
        (Smalltalk at: #LibPTerm) class
            compile: (((Smalltalk at: #LibPTerm) class sourceCodeAt: #source)
                copyReplaceAll: 'ioctl(0, TIOCSCTTY, 1);' with: '') ] ].
(Smalltalk at: #LibPTerm) compile.
(Smalltalk at: #TerminalEmulator) open: fileReference fullName arguments: arguments.
[
    (Delay forSeconds: 15) wait.
    LibC system: 'kill -s KILL $PPID'
] fork
lxsang commented 10 months ago

In the ‘master’ version, ‘pterm_spawn_tty’ has been removed, its call of ‘setsid’ was replicated by the use of ‘POSIX_SPAWN_SETSID’ in #spawnAttrSetting but I’m not sure the call of ‘ioctl’ with ‘TIOCSCTTY’ was replicated.

I don't think it was replicated and i don't see how to replicate it with posix_spawn{p}, as the IO command shall be called in the child process...

Rinzwind commented 10 months ago

I don’t see a way of replicating it either, other than by using a wrapper utility.

I tried: using the version of PTerm in commit 0974fb3f21, I removed the ‘ioctl’ call from ‘pterm_spawn_tty’, and changed the assignment to ‘term’ in TerminalEmulator’s #open:arguments: method to:

term := PTerm new
    xspawn: { (FileLocator imageDirectory / 'setctty') fullName. path. path } , arguments
    env: self environ.

I implemented the wrapper utility ‘setctty’ as:

#include <stdio.h>
#include <unistd.h>
#include <sys/ioctl.h>

int main(int argc, char *argv[]) {
    if (argc < 2) {
        fprintf(stderr, "Too few arguments given\n");
        return 1;
    }
    if (ioctl(0, TIOCSCTTY, 1) == -1) {
        perror("Could not set controlling terminal");
        return 1;
    }
    execvp(argv[1], &argv[2]);
    perror("Could not execute command");
    return 1;
}

Then, when killing Pharo with an open Terminal window running ‘siglog’, the ‘siglog’ process is sent SIGHUP.

I tried that using the ‘master’ version of PTerm too: I again changed the assignment to ‘term’ in TerminalEmulator’s #open:arguments: to use ‘setctty’ as a wrapper. But when killing Pharo with an open Terminal window running ‘siglog’, the ‘siglog’ process is still not sent SIGHUP. I’m not sure what else is still needed.

In any case, using a wrapper utility reintroduces the need to compile C code. Perhaps we should instead consider proposing adding a VM primitive. The plugin that the original Squeak goodie used is still in the ‘PseudoTTYPlugin’ directory in the ‘opensmalltalk-vm’ repository and could perhaps be included in the Pharo VM again as well. But the plugin seems to offer functionality that PTerm no longer needs, just having ‘pterm_spawn_tty’ as a primitive would seem sufficient.

Rinzwind commented 6 months ago

I opened pharo-vm pull request #742 to propose adding a library with a function like ‘pterm_spawn_tty’ to the VM.