Problem:
1. Log messages (especially in CI) are hard to correlate with tests.
2. Since b353a5c05f#11886, dumplog() prints the logs next to test
failures. This is noisy and gets in the way of the test results.
Solution:
1. Associate an incrementing id with each test and include it in log
messages.
- FUTURE: add v:name so Nvim instances can be formally "named"?
2. Mention "child" in log messages if the current Nvim is a child (based
on the presence of $NVIM).
BEFORE:
DBG … 12345 UI: event
DBG … 12345 log_server_msg:722: RPC ->ch 1: …
DBG … 12345 UI: flush
DBG … 12345 inbuf_poll:444: blocking... events_enabled=1 events_pending=0
DBG … 23454 UI: stop
INF … 23454 os_exit:594: Nvim exit: 0
AFTER:
DBG … T57 UI: event
DBG … T57 log_server_msg:722: RPC ->ch 1: …
DBG … T57 UI: flush
DBG … T57 inbuf_poll:444: blocking... events_enabled=1 events_pending=0
DBG … T57/child UI: stop
INF … T57/child os_exit:594: Nvim exit: 0
* refactor: format with uncrustify
* fixup(dundar): fix functions comments
* fixup(dundar): remove space between variable and ++/--
* fixup(dundar): better workaround for macro attributes
This is done to be able to better use uncrustify rules for macros
* fixup(justin): make preprocessors follow neovim style guide
Before #12937, the only way to specify the `$TERM` for a pty job was
through the `TERM` key in the job's opts dict. This was shuttled to the
child process throug a special field on the PtyProcess object and
injected into the environment after forking.
Now that we have a proper way to specify the environment for a job, we
can simply ensure that the env dict has a proper `TERM` set and avoid
the extra shuttling of data around.
This deprecates the use of the `TERM` option, but will still honor it if
present, although at a lower priority than a `TERM` present in the env
dict.
This also fixes#13874 because we're no longer trying to overwrite
`TERM` in the env dict with the special pty `term_name`. Doing so
raises an internal error because of the existing key which, under
certain circumstances, would cause the "hit enter" prompt. However,
since the child process had already forked, there was no way for the
user to acknowledge the prompt and we would just hang there.
Binding _NSGetEnviron()'s return value to a local variable and then
re-binding that is incorrect. We need to directly update what
_NSGetEnviron() refers to.
When starting a pty job, there are certain env vars that we need to
either add or remove. Currently, there are two relevant scenarios.
* Removing irrelevant env vars on Unix, mostly related to the terminal
hosting nvim since they do not apply to a libvterm-hosted terminal.
* Adding required env vars for Windows jobs.
- Move __gcov_flush to process_spawn, for more reliable coverage
tracking of subprocesses
- Travis: use GCOV_ERROR_FILE
- codecov: use "-X fix" to skip "fixing" uploaded coverage data; it
should be handled by codecov's backend instead.
- AppVeyor: no $PATH mangling, which breaks with the improved coverage tracking
due to missing .dll in PATH.
This restores missing coverage again.
Move it to process_spawn in os/pty_process_unix.c, since it seems to
break printargs-test on Windows/AppVeyor otherwise (#10248).
- Minimum required libuv is now v1.12
- Because `uv_os_getenv` requires allocating, we must manage a map
(`envmap` in `env.c`) to maintain the old behavior of `os_getenv` .
- free() map-items after removal. khash.h does not make copies of
anything, so even its keys must be memory-managed by the caller.
closes#8398closes#9267
If a second and third child exit while we are already in the handler, we
will only see a single additional SIGCHLD. Therefore the handler must
not stop after processing a single child but should check all children.
Fixes#8740
It seems as though in an AppImage there's an extra child process that
dies at some early point, before we have set up a SIGCHLD handler. So
when we later get a SIGCHLD from a child that we do care about,
waitpid(-1, ...) tells us about the extra child - and we don't notice
that the interesting child has exited.
Or something like that!
See also:
* https://patchwork.kernel.org/patch/9949491/ in which perf hit
something similar
* discussion at the AppImage repository:
https://github.com/AppImage/AppImageKit/issues/812#issuecomment-404662110.
Fix is to be explicit about which process we are waitpid()'ing for, so
we never need be distracted by children that we don't know about.
Fixes#8104
UV_PROCESS_DETACHED compels libuv:uv__process_child_init() to call
setsid() in the child just after fork(). That ensures the process and
its descendants are grouped in a separate session (and process group).
The following jobstart() call correctly groups `sh` and `sleep` in a new
session (and process-group), where `sh` is the "session leader" (and
process-group leader):
:call jobstart(['sh','-c','sleep 60'])
SESN PGRP PID PPID Command
30383 30383 30383 3620 │ ├─ -bash
30383 31432 31432 30383 │ │ └─ nvim -u NORC
30383 31432 31433 30383 │ │ ├─ nvim -u NORC
8105 8105 8105 31432 │ │ └─ sh -c sleep 60
8105 8105 8106 8105 │ │ └─ sleep 60
closes#6530
ref: https://stackoverflow.com/q/1046933
ref: https://unix.stackexchange.com/a/404065
Helped-by: Marco Hinz <mh.codebro+github@gmail.com>
Discussion
------------------------------------------------------------------------
On my linux box before this patch, the termclose_spec.lua:'kills job
trapping SIGTERM' test indirectly causes cmake/busted to wait for 60s.
That's because the test spawns a `sleep 60` descendant process which
hangs around even after nvim exits: nvim killed the parent PID, but not
PGID (process-group), so the grandchild "reparented" to init (PID 1).
Session contains processes (and process-groups) which are logically part
of the same "login session". Process-group is a set of
logically/informally-related processes within a session; for example,
shells assign a process group to each "job". Session IDs and PGIDs both
have type pid_t (like PIDs).
These OS-level mechanisms are, as usual, legacy accidents whose purpose
is upheld by convention and folklore. We can use session-level grouping
(setsid), or we could use process-group-level grouping (setpgid).
Vim uses setsid() if available, otherwise setpgid(0,0).
Windows
------------------------------------------------------------------------
UV_PROCESS_DETACHED on win32 sets CREATE_NEW_PROCESS_GROUP flag.
But uv_kill() does not kill the process-group:
https://github.com/nodejs/node/issues/3617
Ideas:
- Set UV_PROCESS_DETACHED (CREATE_NEW_PROCESS_GROUP), then call
GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, pid)
- Maybe won't work because MSDN says "Only processes that share the
same console as the calling process receive the signal."
https://docs.microsoft.com/en-us/windows/console/generateconsolectrlevent
But CREATE_NEW_PROCESS_GROUP creates a new console ...
ref https://stackoverflow.com/q/1453520
- Group processes within a "job". libuv does that *globally* for
non-detached processes: uv__init_global_job_handle.
- Iterate through CreateToolhelp32Snapshot().
- https://stackoverflow.com/q/1173342
- Vim does this, see terminate_all()
On startup, if running in a terminal, save the termios properties.
Use the saved termios for `:terminal` and `jobstart()` pty jobs.
This won't affect nvim spawned outside of a terminal.
questions:
- This affects `:terminal` and `jobstart({'pty'✌️true})`.
Should we be more conservative for `jobstart({'pty'✌️true})` (e.g.
pass NULL to forkpty() and let the OS defaults prevail)?
- Note: `iutf8` would not be set in that case.
Processes in vim are always started in the current directory, which
causes issues when the process is a daemon and the current directory is
a mountpoint. Fix this by adding an option to set the cwd of the new
process with jobstart(). In addition, fix termopen() so that it actually
uses the cwd option from the dict (it couldn't previously set the cwd
value due to dead code).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
