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

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

Closed
Rinzwind opened this issue Oct 4, 2023 · 5 comments

Comments

@Rinzwind
Copy link
Collaborator

Rinzwind commented Oct 4, 2023

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
Copy link
Owner

lxsang commented Oct 9, 2023

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
Copy link
Collaborator Author

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
Copy link
Owner

lxsang commented Oct 23, 2023

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
Copy link
Collaborator Author

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 0974fb3, 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
Copy link
Collaborator Author

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

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

No branches or pull requests

2 participants