Problem:
When stopping a PTY process on Windows, the exit code indicates that the
process is stopped by SIGTERM even when closing all streams is enough to
terminate the process. This is inconsistent with other platforms.
Solution:
Set exit_signal to SIGHUP instead of SIGTERM when using SIGHUP.
This reverts commit ab8371a26c.
Need to think of a different solution, which may require adding new
flags to nvim_ui_attach() (e.g. passing stdout or stderr fd).
using the GNU compiler we just get a bunch of const warnings we can fix.
clang, however, gets really upset that the standard library suddenly
starts using a lot of c11 features, despite us being in -std=gnu99 mode.
Basically, _GNU_SOURCE which we set is taken as a _carte blanche_ by the
glibc headers to do whatever they please, and thus we must inform clang
that everything is still OK.
Problem: If Nvim server fails to --listen and prints error before the
TUI enters alternate screen, the error isn't visible.
Solution: Forward server stderr using client side stderr handler instead
of having the server inherit client stderr file descriptor.
This does mean that `stderr_isatty` will be `false` in the server, but
that value doesn't matter in embedded mode.
Always pass `stdin_fd` to embedded server to avoid a hang when reading
from stdin when it's a TTY (not sure why one wants to do that, perhaps
by mistake), because if `stdin_fd` isn't passed, the server will try to
use stderr as stdin.
Example test failure on CI:
FAILED test/functional/terminal/tui_spec.lua @ 41: TUI exit status 1 and error message with server --listen error #34365
test/functional/terminal\tui_spec.lua:55: Failed to match any screen lines.
Expected (anywhere): "nvim%.exe: Failed to %-%-listen: address already in use:"
Actual:
|{114:nvim.exe -h"} |
| |
|[Process exited 1]^ |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
|{5:-- TERMINAL --} |
Snapshot:
screen:expect([[
{114:nvim.exe -h"} |
|
[Process exited 1]^ |
|*13
{5:-- TERMINAL --} |
]])
stack traceback:
test\functional\ui\screen.lua:909: in function '_wait'
test\functional\ui\screen.lua:537: in function 'expect'
test/functional/terminal\tui_spec.lua:55: in function <test/functional/terminal\tui_spec.lua:41>
In this case, it appears that the client entered alternate screen in the
middle of the server's print_mainerr().
It is indeed possible that a signal may arrive during flush_stream()
(e.g. SIGCHLD from another child process), so poll() again on EINTR.
Fixes the following Coverity warning:
_____________________________________________________________________________________________
*** CID 644345: Error handling issues (CHECKED_RETURN)
/src/nvim/event/proc.c: 405 in flush_stream()
399
400 #ifdef __linux__
401 // On Linux, libuv's polling (which uses epoll) doesn't flush PTY master's pending
402 // work on kernel workqueue, so use an explcit poll() before that. #37982
403 if (proc->type == kProcTypePty && !stream->did_eof) {
404 struct pollfd pollfd = { .fd = ((PtyProc *)proc)->tty_fd, .events = POLLIN };
>>> CID 644345: Error handling issues (CHECKED_RETURN)
>>> Calling "poll(&pollfd, 1UL, 0)" without checking return value. This library function may fail and return an error code.
405 poll(&pollfd, 1, 0);
406 }
407 #endif
408 // Poll for data and process the generated events.
409 loop_poll_events(proc->loop, 0);
410 if (stream->s.events) {
Another possible error is ENOMEM, which is probably not worth handling.
For reference, #23308 previously removed a case of ENOMEM handling, and
this PR also removes an outdated mention of that.
Also reduce the number of #ifdefs in non-OS-specific files.
Problem: Terminal doesn't detect if the PTY process is suspended or
offer a convenient way for the user to resume the process.
Solution: Detect suspended PTY process on SIGCHLD and show virtual text
"[Process suspended]" at the bottom-left. Resume the process
when the user presses a key.
Problem:
Crashed Nvim leaves socket files. Subsequent starts fail with "address already in use".
Solution:
On bind failure, test socket liveness via connect(). Remove if stale, retry bind().
Fixes#36581
Using uv_pipe_t to read from PTY master may drop data if the PTY process
exits immediately after output, as libuv treats a partial read after
POLLHUP as EOF, which isn't true for PTY master on Linux. Therefore use
uv_poll_t instead.
This commit can be reverted if libuv/libuv#4992 is fixed for uv_pipe_t.
On linux /dev/stdin is defined as a symlink to /proc/self/fd/0
This in turn is defined as a "magic" symlink which is allowed to point
to internal kernel objects which really does not have a file
name. As a glaring inconsistency, fopen("/proc/self/fd/0", "r")
works if fd was originally opened using pipe() but not using
socketpair(). As it happens UV_CREATE_PIPE does not create pipes
but creates socket pairs. These two unfortunate conditions
means that using /dev/stdin and similar does not work in
shell commands in nvim on linux. as a work around, override
libuv's descicion and create an actual pipe pair.
This change is not needed on BSD:s but done unconditionally for simplicity,
except for on windows where it is not done for stdout because of windows
fixes#35984
Problem:
In socket_connect(), if connecting to the given TCP address times out,
libuv is still trying to connect to the address, and connect_cb may be
called when running the libuv event loop after the `status` variable
referenced by `req.data` goes out of scope.
Solution:
Close the uv_tcp_t handle and wait for connect_cb to be called before
retrying or failing in socket_connect(). This also avoid leaking libuv
handles.
The tests added here only check that the non-timeout case still works,
as checking the timeout case is very hard without modifications to the
code. Removing the first LOOP_PROCESS_EVENT_UNTIL() in socket_connect()
(the one with the timeout) is a way to check that manually.
Also add a comment about the cause of the ASAN error in #34586.
These are not needed after #35129 but making uncrustify still play nice
with them was a bit tricky.
Unfortunately `uncrustify --update-config-with-doc` breaks strings
with backslashes. This issue has been reported upstream,
and in the meanwhile auto-update on every single run has been disabled.
Uses the undocumented "error_exit" UI event for a different purpose:
When :detach is used on the server, send an "error_exit" with 0 `status`
to indicate that the server shouldn't wait for client exit.
Problem:
On Windows, since b360c06085, UI may hang on exit. #33019
Solution:
Restore the hack in on_proc_exit, until we can figure out why rpc_close
is not called in the UI client when the server closes the channel.
Problem:
`receive_msgpack()` calls `channel_close()` just before calling
`chan_close_on_err()`, even though `chan_close_on_err()` always calls
`channel_close()`.
Solution:
Remove the redunant `channel_close()` in `receive_msgpack()`.
Problem:
When a function like vim.wait() is used, we continuously drain the main
event queue until it is empty, never stopping for user input. This means
the libuv timer never runs and the terminal never gets refreshed, so
emit_termrequest continously reschedules itself onto the same event
queue, causing an infinite loop.
Solution:
Use a separate "pending" event queue, where events that require a
terminal refresh are temporarily placed. Drain this queue after a
terminal refresh and events are copied back onto the main queue. This
prevents infinite loops since the main event queue will always be able
to properly drain.
Problem:
on_proc_exit() has a special-case that assumes that the UI client will
never spawn more than 1 child process.
Solution:
If the Nvim server exits, the stream EOF will trigger `rpc_close()` in
the UI client, so we don't need the special case in `on_proc_exit`.
Pass `Channel.exit_status` from `rpc_close()` so that the correct exit
code is reflected.
Problem:
On Windows, spawning the `nvim --embed` server with `detach=true` breaks
various `tt.setup_child_nvim` tests.
Solution:
Make this behavior opt-in with an env var, temporarily.
loop->children might have been a linked list because used to be
modified in place while looped over. However the loops that exists
rather schedules events to be processed later, outside of the loop,
so this can not happen anymore.
When a linked list is otherwise useful it is better to use
lib/queue_defs.h which defines an _intrusive_ linked list (i e it
doesn't need to do allocations for list items like klist ).
Problem:
- "process" is often used as a verb (`multiqueue_process_events`), which
is ambiguous for cases where it's used as a topic.
- The documented naming convention for processes is "proc".
- `:help dev-name-common`
- Shorter is better, when it doesn't harm readability or
discoverability.
Solution:
Rename "process" => "proc" in all C symbols and module names.
Problem:
The name `os_inchar` (from Vim's old `mch_inchar`) is ambiguous:
"inchar" sounds like it could be reading or enqueuing (setting) input.
Its docstring is also ambiguous.
Solution:
- Rename `os_inchar` to `input_get`.
- Write some mf'ing docstrings.
- Add assert() in TRY_READ().
If you like it you shouldn't put a ring on it.
This is what _every_ consumer of RStream used anyway, either by calling
rbuffer_reset, or rbuffer_consumed_compact (same as rbuffer_reset
without needing a scratch buffer), or by consuming everything in
each stream_read_cb call directly.
This is a structural refactor with no logical changes, yet. Done in
preparation for simplifying rstream/rbuffer which will require more
state inline in RStream.
The initial idea was to have RStream and WStream as sub-types
symetrically but that doesn't work, as sockets are both reading and
writing. Also there is very little write-specific state to start with,
so the benefit of a separate WStream struct is a lot smaller. Just
document what fields in `Stream` are write specific.
Also close Nvim instance before removing log file, otherwise the Nvim
instance will still write to the log file.
Also adjust log level in libuv_process_spawn(). Ref #27660
Remove `export` pramgas from defs headers as it causes IWYU to believe
that the definitions from the defs headers comes from main header, which
is not what we really want.
