TUI: system(), systemlist(), :! in same process-group

[justinmk]

#8217 and #8450 describe some casualties of the change in #8107: since #8107, all processes spawned by Nvim are created in their own session (by calling setsid() in the child). Using setsid() avoids serious bugs like #6530 (orphaned processes). It's a good thing to do, and is essentially what Vim's job_start() does as well.

However, the terminal version (at least) of Vim doesn't do that for system(), systemlist() and :!.

"Fixing" this as attempted in #8389 would regress #8107 for system(), systemlist(), and :!. For example, CTRL-C during this command would leave orphan processes:

:echo system('sleep 30|sleep 30|sleep 30')

Vim's approach for system(), systemlist(), and :! is:

1. Block signals
2. fork()
3. Unblock signals in the child
4. On CTRL-C, send SIGINT to the process-group id
5. The parent (vim) ignores the signal. The child and its descendants do not, so sending the signal to the group kills everything except vim itself.

To do this in Nvim will (I think) require adding a case in process_spawn() to work without using libuv. This could be implemented like how src/nvim/os/pty_process_unix.c implements ProcessType.kProcessTypePty: add ProcessType.kProcessTypeUnix and special-case it.

The work will be in implementing src/nvim/os/unix_process.c (again as mentioned, src/nvim/os/pty_process_unix.c would be a good starting point).

[kghost]

Currently both neovim and vim is doing wrong. To fix the problem once for all, use the following process:

1. neovim prepares pipes for the new chile
2. neovim forks a new child process
3. child dups pipes to stdin/stdout
4. both neovim and child: create a new process group for the child (setpgid)
5. both neovim and child: set the child active process group (tcsetpgrp)
6. exec in the child (sh -c "......")
7. while neovim waits child, handle input/output of child via pipes
8. child exits, neovim is notified by SIGCLD
9. set neovim to active process group (again tcsetpgrp)

Handle signals

Here is how we handle signals during step 7. As we already set the child as active process group, when user hit Ctrl-C, the SIGINT will be send to the active process group directly (aka, all process created by the command), this is the default pty behaviour. And it is the child's duty to clean up all his children. In most case, all children created will receive SIGINT from tty, and quit themself, if some processes refuse to exit by SIGINT, it is their choice, nothing related to neovim.

Note

1. no session is needed (setsid), unless we are emulating a term. So only termopen should create a new session, no other command should create a new session.
2. the signals are generated and delivered by tty, we should avoid to proxy/delegate signals.
3. when neovim decided to quit before some of its children, instead of use SIGTERM, we should sue SIGHUP to kill all the child process groups.

[justinmk]

I haven't explained what's wrong, but #8450 #8217 #8527 does

At least #8217 can be solved by changing :! to use :terminal implicitly (described in #5431). That is the long-term solution, and would be much, much more valuable.

I'll reopen this since I forgot about the workaround in the main description:

adding a case in process_spawn() to work without using libuv.

But really, the long-term solution is to use :terminal implementation for :!. Nvim is a client-server architecture first. The terminal UI is just a UI.

[kghost]

OK, I got your concept, and happy to progress this way.

Currently uv_spawn is designed for detach process, which do use setsid but doesn't allocate pty. So I'm finding a way to replace it as you suggested,

adding a case in process_spawn() to work without using libuv.

[qbedard]

Seems like this might be the cause of this: kovidgoyal/kitty#4869

Can anyone help me confirm? Any news on a fix?

[laktak]

Would this fix the issue that the FAQ describes as

! AND SYSTEM() DO WEIRD THINGS WITH INTERACTIVE PROCESSES

I use this a lot in Vim to launch TUIs (file manager, git, etc.) and would like to have this available in Neovim as well.

[zeertzjq]

No