This fixes a regression from cf6f60ce4d
(possibly), as before that commit a frame is popped from the call stack
immediately after its response is received.
Also fix leaking the allocated error messages.
Problem:
MessagePack fixstr format supports string lengths 0-31, but mpack_str()
only used fixstr for lengths < 20. Strings of 20-31 bytes were
incorrectly encoded as str8 (2-byte header) instead of fixstr (1-byte
header).
Solution:
Change the condition from `len < 20` to `len < 32` to match the
MessagePack specification.
This fix affects message timing which exposed a pre-existing race
condition in the channels_spec PTY test. The test now uses a helper
function to accumulate partial PTY reads, making it more robust.
Fixes#32784
Problem:
On MacOS it is a relatively common pattern to set XDG_RUNTIME_DIR under
`/tmp` or `/var`. Both of these are symlinks to `/private/tmp` and
`/private/var`. When checking for loopback the input address is
normalized using `fix_fname`, however this is not applied to the
addresses of the sockets. As a result of one address being normalized
and the other not the comparison would fail.
Solution:
Normalize both sides of the comparison using `fix_fname`.
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.
Using the preprocessor before generating prototypes provides some
"niceties" but the places that rely on these are pretty few.
Vastly simplifying the BUILD SYSTEM is a better trade-off.
Unbalancing { } blocks due to the preprocessor is cringe anyway (think
of the tree-sitter trees!), write these in a different way.
Add some workarounds for plattform specific features.
INCLUDE_GENERATED_DECLARATIONS flag is now technically redundant,
but will be cleaned up in a follow-up PR as it is a big mess.
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.
When the TUI is suspending or stopping, redraw events should not be
processed, as when it next processes redraw events it's already waiting
for a DA1 response after having disabled some terminal modes.
Fix#33708
Problem:
vim.uv.os_setenv gets "stuck" per-key. #32550
Caused by the internal `envmap` cache. #7920
:echo $FOO <-- prints nothing
:lua vim.uv.os_setenv("FOO", "bar")
:echo $FOO <-- prints bar (as expected)
:lua vim.uv.os_setenv("FOO", "fizz")
:echo $FOO <-- prints bar, still (not expected. Should be "fizz")
:lua vim.uv.os_unsetenv("FOO")
:echo $FOO <-- prints bar, still (not expected. Should be nothing)
:lua vim.uv.os_setenv("FOO", "buzz")
:echo $FOO <-- prints bar, still (not expected. Should be "buzz")
Solution:
- Remove the `envmap` cache.
- Callers to `os_getenv` must free the result.
- Update all call-sites.
- Introduce `os_getenv_noalloc`.
- Extend `os_env_exists()` the `nonempty` parameter.
Problem:
chan_close_on_err() writes to the log file before peforming its actual
work. This could be slightly misleading in terms of log timestamps, or
could delay important work on Windows where file-open/write can be slow.
Solution:
Log after closing the channel, instead of before.
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()`.
When a UI detaches it will execute any UILeave events. Autocommands
cannot run in a libuv handler because they will in turn poll the event
loop, which results in recursive loop execution. So we schedule the
callback to detach the UI on the main event queue.
We also have to schedule the exit when the RPC channel is closed to
ensure it does not run until after `remote_ui_disconnect` has run,
otherwise it will hang.
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.
In the api_info() output:
:new|put =map(filter(api_info().functions, '!has_key(v:val,''deprecated_since'')'), 'v:val')
...
{'return_type': 'ArrayOf(Integer, 2)', 'name': 'nvim_win_get_position', 'method': v:true, 'parameters': [['Window', 'window']], 'since': 1}
The `ArrayOf(Integer, 2)` return type didn't break clients when we added
it, which is evidence that clients don't use the `return_type` field,
thus renaming Dictionary => Dict in api_info() is not (in practice)
a breaking change.
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().
Problem:
If $NVIM_APPNAME is a relative dir path, Nvim fails to start its
primary/default server, and `v:servername` is empty.
Root cause is d34c64e342, but this wasn't
noticed until 96128a5076 started reporting the error more loudly.
Solution:
- `server_address_new`: replace slashes "/" in the appname before using
it as a servername.
- `vim_mktempdir`: always prefer the system-wide top-level "nvim.user/"
directory. That isn't intended to be specific to NVIM_APPNAME; rather,
each *subdirectory* ("nvim.user/xxx") is owned by each Nvim instance.
Nvim "apps" can be identified by the server socket(s) stored in those
per-Nvim subdirs.
fix#30256
Problem:
$XDG_RUNTIME_DIR may be broken on WSL, which prevents starting (and even
building) Nvim. #30282
Solution:
- When startup fails, mention the servername in the error message.
- If an autogenerated server address fails, log an error and continue
with an empty `v:servername`. It's only fatal if a user provides a bad
`--listen` or `$NVIM_LISTEN_ADDRESS` address.
Before:
$ nvim --headless --listen ./hello.sock
nvim: Failed to --listen: "address already in use"
$ NVIM_LISTEN_ADDRESS='./hello.sock' ./build/bin/nvim --headless
nvim: Failed to --listen: "address already in use"
After:
$ nvim --headless --listen ./hello.sock
nvim: Failed to --listen: address already in use: "./hello.sock"
$ NVIM_LISTEN_ADDRESS='./hello.sock' ./build/bin/nvim --headless
nvim: Failed $NVIM_LISTEN_ADDRESS: address already in use: "./hello.sock"
Problem:
`nvim --listen` does not error on EADDRINUSE. #30123
Solution:
Now that `$NVIM_LISTEN_ADDRESS` is deprecated and input *only* (instead
of the old, ambiguous situation where it was both an input *and* an
output), we can be fail fast instead of trying to "recover". This
reverts the "recovery" behavior of
704ba4151e, but that was basically
a workaround for the fragility of `$NVIM_LISTEN_ADDRESS`.
This also makes shada reading slightly faster due to avoiding
some copying and allocation.
Use keysets to drive decoding of msgpack maps for shada entries.
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.
Using a ring buffer for buffered synchronous fileio is just unnecessary
complexity.
- when reading, we always consume the _entire_ buffer before getting
into syscalls. Thus we reset the buffer to its initial position before
when we actually read.
- when writing and buffer is full, we always flush the entire buffer
before starting to buffer again. So we can reset the buffer to its
initial state.
Also no static buffers are needed for writing and skipping. Needing an
extra copy for each write completely defeated the purpose of
a ring buffer (if there had been one)
Problem:
`vim.rpcnotify(0)` and `rpcnotify(0)` are documented as follows:
If {channel} is 0, the event is broadcast to all channels.
But that's not actually true. Channels must call `nvim_subscribe` to
receive "broadcast" events, so it's actually "multicast".
- Assuming there is a use-case for "broadcast", the current model adds
an extra step for broadcasting: all channels need to "subscribe".
- The presence of `nvim_subscribe` is a source of confusion for users,
because its name implies something more generally useful than what it
does.
Presumably the use-case of `nvim_subscribe` is to avoid "noise" on RPC
channels not expected a broadcast notification, and potentially an error
if the channel client reports an unknown event.
Solution:
- Deprecate `nvim_subscribe`/`nvim_unsubscribe`.
- If applications want to multicast, they can keep their own multicast
list. Or they can use `nvim_list_chans()` and `nvim_get_chan_info()`
to enumerate and filter the clients they want to target.
- Always send "broadcast" events to ALL channels. Don't require channels
to "subscribe" to receive broadcasts. This matches the documented
behavior of `rpcnotify()`.
Before, we needed to always pack an entire msgpack_rpc Object to
a continous memory buffer before sending it out to a channel.
But this is generally wasteful. it is better to just flush
whatever is in the buffer and then continue packing to a new buffer.
This is also done for the UI event packer where there are some extra logic
to "finish" of an existing batch of nevents/ncalls. This doesn't really
stop us from flushing the buffer, just that we need to update the state
machine accordingly so the next call to prepare_call() always will
start with a new event (even though the buffer might contain overflow
data from a large event).
As only a few API functions make use of explicit freeing of the return
value, make it opt-in instead. The arena is always present under the
hood, so `Arena *arena` arg now doesn't mean anything other than getting
access to this arena. Also it is in principle possible to return an
allocated value while still using the arena as scratch space for other
stuff (unlikely, but there no reason to not allow it).
Note: kSDItemHeader is something is _written_ by nvim in the shada file
to identify it for debugging purposes outside of nvim. But this data wasn't ever used by
neovim after reading the file back, So I removed the parsing of it for now.
Note: this contains two _temporary_ changes which can be reverted
once the Arena vs no-Arena distinction in API wrappers has been removed.
Both nlua_push_Object and object_to_vim_take_luaref() has been changed
to take the object argument as a pointer. This is not going to be
necessary once these are only used with arena (or not at all) allocated
Objects.
The object_to_vim() variant which leaves luaref untouched might need to
stay for a little longer.
and for return value of nlua_exec/nlua_call_ref, as this uses
the same family of functions.
NB: the handling of luaref:s is a bit of a mess.
add api_luarefs_free_XX functions as a stop-gap as refactoring
luarefs is a can of worms for another PR:s.
as a minor feature/bug-fix, nvim_buf_call and nvim_win_call now preserves
arbitrary return values.
