From 4fb48c5654d9ffbffbdcdd80d0498b1ea3c8e68b Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 3 May 2022 15:08:35 +0200 Subject: feat(server): set $NVIM, unset $NVIM_LISTEN_ADDRESS #11009 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PROBLEM ------------------------------------------------------------------------ $NVIM_LISTEN_ADDRESS has conflicting purposes as both a parameter ("the current process should listen on this address") and a descriptor ("the current process is a child of this address"). This contradiction means the presence of NVIM_LISTEN_ADDRESS is ambiguous, so child Nvim always tries to listen on its _parent's_ socket. This is the cause of lots of "Failed to start server" spam in our test/CI logs: WARN 2022-04-30… server_start:154: Failed to start server: address already in use: \\.\pipe\nvim-4480-0 WARN 2022-04-30… server_start:154: Failed to start server: address already in use: \\.\pipe\nvim-2168-0 SOLUTION ------------------------------------------------------------------------ 1. Set $NVIM to the parent v:servername, *only* in child processes. - Now the correct way to detect a "parent" Nvim is to check for $NVIM. 2. Do NOT set $NVIM_LISTEN_ADDRESS in child processes. 3. On startup if $NVIM_LISTEN_ADDRESS exists, unset it immediately after server init. 4. Open a channel to parent automatically, expose it as v:parent. Fixes #3118 Fixes #6764 Fixes #9336 Ref https://github.com/neovim/neovim/pull/8247#issuecomment-380275696 Ref #8696 --- src/nvim/msgpack_rpc/server.c | 49 +++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 28 deletions(-) (limited to 'src/nvim/msgpack_rpc/server.c') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index e954e4b3a3..f15ce82917 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -22,7 +22,8 @@ #include "nvim/vim.h" #define MAX_CONNECTIONS 32 -#define LISTEN_ADDRESS_ENV_VAR "NVIM_LISTEN_ADDRESS" +#define ENV_LISTEN "NVIM_LISTEN_ADDRESS" // deprecated +#define ENV_NVIM "NVIM" static garray_T watchers = GA_EMPTY_INIT_VALUE; @@ -35,20 +36,24 @@ bool server_init(const char *listen_addr) { ga_init(&watchers, sizeof(SocketWatcher *), 1); - // $NVIM_LISTEN_ADDRESS - const char *env_addr = os_getenv(LISTEN_ADDRESS_ENV_VAR); - int rv = listen_addr == NULL ? 1 : server_start(listen_addr); + // $NVIM_LISTEN_ADDRESS (deprecated) + if (!listen_addr && os_env_exists(ENV_LISTEN)) { + listen_addr = os_getenv(ENV_LISTEN); + } + int rv = listen_addr ? server_start(listen_addr) : 1; if (0 != rv) { - rv = env_addr == NULL ? 1 : server_start(env_addr); - if (0 != rv) { - listen_addr = server_address_new(); - if (listen_addr == NULL) { - return false; - } - rv = server_start(listen_addr); - xfree((char *)listen_addr); + listen_addr = server_address_new(); + if (!listen_addr) { + return false; } + rv = server_start(listen_addr); + xfree((char *)listen_addr); + } + + if (os_env_exists(ENV_LISTEN)) { + // Unset $NVIM_LISTEN_ADDRESS, it's a liability hereafter. + os_unsetenv(ENV_LISTEN); } return rv == 0; @@ -60,8 +65,8 @@ static void close_socket_watcher(SocketWatcher **watcher) socket_watcher_close(*watcher, free_server); } -/// Set v:servername to the first server in the server list, or unset it if no -/// servers are known. +/// Sets the "primary address" (v:servername and $NVIM) to the first server in +/// the server list, or unsets if no servers are known. static void set_vservername(garray_T *srvs) { char *default_server = (srvs->ga_len > 0) @@ -156,12 +161,6 @@ int server_start(const char *endpoint) return result; } - // Update $NVIM_LISTEN_ADDRESS, if not set. - const char *listen_address = os_getenv(LISTEN_ADDRESS_ENV_VAR); - if (listen_address == NULL) { - os_setenv(LISTEN_ADDRESS_ENV_VAR, watcher->addr, 1); - } - // Add the watcher to the list. ga_grow(&watchers, 1); ((SocketWatcher **)watchers.ga_data)[watchers.ga_len++] = watcher; @@ -200,12 +199,6 @@ bool server_stop(char *endpoint) return false; } - // Unset $NVIM_LISTEN_ADDRESS if it is the stopped address. - const char *listen_address = os_getenv(LISTEN_ADDRESS_ENV_VAR); - if (listen_address && STRCMP(addr, listen_address) == 0) { - os_unsetenv(LISTEN_ADDRESS_ENV_VAR); - } - socket_watcher_close(watcher, free_server); // Remove this server from the list by swapping it with the last item. @@ -215,8 +208,8 @@ bool server_stop(char *endpoint) } watchers.ga_len--; - // If v:servername is the stopped address, re-initialize it. - if (STRCMP(addr, get_vim_var_str(VV_SEND_SERVER)) == 0) { + // Bump v:servername to the next available server, if any. + if (strequal(addr, (char *)get_vim_var_str(VV_SEND_SERVER))) { set_vservername(&watchers); } -- cgit From 8f065205946844d87f00d6c55517521e3809f821 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 23 May 2022 21:44:15 -0700 Subject: feat(logging): include test-id in log messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: 1. Log messages (especially in CI) are hard to correlate with tests. 2. Since b353a5c05f02 #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 --- src/nvim/msgpack_rpc/server.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'src/nvim/msgpack_rpc/server.c') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index f15ce82917..3ced39117a 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -23,7 +23,6 @@ #define MAX_CONNECTIONS 32 #define ENV_LISTEN "NVIM_LISTEN_ADDRESS" // deprecated -#define ENV_NVIM "NVIM" static garray_T watchers = GA_EMPTY_INIT_VALUE; @@ -56,6 +55,11 @@ bool server_init(const char *listen_addr) os_unsetenv(ENV_LISTEN); } + // TODO: this is for logging_spec. Can remove this after nvim_log #7062 is merged. + if (os_env_exists("__NVIM_TEST_LOG")) { + ELOG("test log message"); + } + return rv == 0; } -- cgit From 1f2c2a35ad14cfac002d87073471bd84a52860bf Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Wed, 1 Jun 2022 11:28:14 -0700 Subject: feat(server): instance "name", store pipes in stdpath(state) Problem: - Unix sockets are created in random /tmp dirs. - /tmp is messy, unclear when OSes actually clear it. - The generated paths are very ugly. This adds friction to reasoning about which paths belong to which Nvim instances. - No way to provide a human-friendly way to identify Nvim instances in logs or server addresses. Solution: - Store unix sockets in stdpath('state') - Allow --listen "name" and serverstart("name") to given a name (which is appended to a generated path). TODO: - is stdpath(state) the right place? --- src/nvim/msgpack_rpc/server.c | 67 ++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 32 deletions(-) (limited to 'src/nvim/msgpack_rpc/server.c') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index 3ced39117a..c9e707aa92 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -42,7 +42,7 @@ bool server_init(const char *listen_addr) int rv = listen_addr ? server_start(listen_addr) : 1; if (0 != rv) { - listen_addr = server_address_new(); + listen_addr = server_address_new(NULL); if (!listen_addr) { return false; } @@ -55,7 +55,7 @@ bool server_init(const char *listen_addr) os_unsetenv(ENV_LISTEN); } - // TODO: this is for logging_spec. Can remove this after nvim_log #7062 is merged. + // TODO(justinmk): this is for logging_spec. Can remove this after nvim_log #7062 is merged. if (os_env_exists("__NVIM_TEST_LOG")) { ELOG("test log message"); } @@ -87,23 +87,26 @@ void server_teardown(void) /// Generates unique address for local server. /// -/// In Windows this is a named pipe in the format -/// \\.\pipe\nvim--. -/// -/// For other systems it is a path returned by vim_tempname(). -/// -/// This function is NOT thread safe -char *server_address_new(void) +/// Named pipe format: +/// - Windows: "\\.\pipe\.." +/// - Other: "~/.local/state/nvim/.." +char *server_address_new(const char *name) { -#ifdef WIN32 static uint32_t count = 0; - char template[ADDRESS_MAX_SIZE]; - snprintf(template, ADDRESS_MAX_SIZE, - "\\\\.\\pipe\\nvim-%" PRIu64 "-%" PRIu32, os_get_pid(), count++); - return xstrdup(template); + char fmt[ADDRESS_MAX_SIZE]; +#ifdef WIN32 + int r = snprintf(fmt, sizeof(fmt), "\\\\.\\pipe\\%s.%" PRIu64 ".%" PRIu32, + name ? name : "nvim", os_get_pid(), count++); #else - return (char *)vim_tempname(); + char *dir = get_xdg_home(kXDGStateHome); + int r = snprintf(fmt, sizeof(fmt), "%s/%s.%" PRIu64 ".%" PRIu32, + dir, name ? name : "nvim", os_get_pid(), count++); + xfree(dir); #endif + if ((size_t)r >= sizeof(fmt)) { + ELOG("truncated server address"); + } + return xstrdup(fmt); } /// Check if this instance owns a pipe address. @@ -118,35 +121,35 @@ bool server_owns_pipe_address(const char *path) return false; } -/// Starts listening for API calls. -/// -/// The socket type is determined by parsing `endpoint`: If it's a valid IPv4 -/// or IPv6 address in 'ip:[port]' format, then it will be a TCP socket. -/// Otherwise it will be a Unix socket or named pipe (Windows). +/// Starts listening for RPC calls. /// -/// If no port is given, a random one will be assigned. +/// Socket type is decided by the format of `addr`: +/// - TCP socket if it looks like an IPv4/6 address ("ip:[port]"). +/// - If [port] is omitted, a random one is assigned. +/// - Unix socket (or named pipe on Windows) otherwise. +/// - If the name doesn't contain slashes it is appended to a generated path. #8519 /// -/// @param endpoint Address of the server. Either a 'ip:[port]' string or an -/// arbitrary identifier (trimmed to 256 bytes) for the Unix -/// socket or named pipe. -/// @returns 0: success, 1: validation error, 2: already listening, -/// -errno: failed to bind or listen. -int server_start(const char *endpoint) +/// @param addr Server address: a "ip:[port]" string or arbitrary name or filepath (max 256 bytes) +/// for the Unix socket or named pipe. +/// @returns 0: success, 1: validation error, 2: already listening, -errno: failed to bind/listen. +int server_start(const char *addr) { - if (endpoint == NULL || endpoint[0] == '\0') { - WLOG("Empty or NULL endpoint"); + if (addr == NULL || addr[0] == '\0') { + WLOG("Empty or NULL address"); return 1; } + bool isname = !strstr(addr, ":") && !strstr(addr, "/") && !strstr(addr, "\\"); + char *addr_gen = isname ? server_address_new(addr) : NULL; SocketWatcher *watcher = xmalloc(sizeof(SocketWatcher)); - - int result = socket_watcher_init(&main_loop, watcher, endpoint); + int result = socket_watcher_init(&main_loop, watcher, isname ? addr_gen : addr); + xfree(addr_gen); if (result < 0) { xfree(watcher); return result; } - // Check if a watcher for the endpoint already exists + // Check if a watcher for the address already exists. for (int i = 0; i < watchers.ga_len; i++) { if (!strcmp(watcher->addr, ((SocketWatcher **)watchers.ga_data)[i]->addr)) { ELOG("Already listening on %s", watcher->addr); -- cgit From f50135a32e11c535e1dc3a8e9460c5b4e640ee86 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 30 Jun 2022 13:16:46 +0200 Subject: feat: stdpath('run'), /tmp/nvim.user/ #18993 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: - Since c57f6b28d71d #8519, sockets are created in ~/.local/… but XDG spec says: "XDG_RUNTIME_DIR: Must be on the local filesystem", which implies that XDG_STATE_DIR is potentially non-local. - Not easy to inspect Nvim-created temp files (for debugging etc). Solution: - Store sockets in stdpath('run') ($XDG_RUNTIME_DIR). - Establish "/tmp/nvim.user/" as the tempdir root shared by all Nvims. - Make ok() actually useful. - Introduce assert_nolog(). closes #3517 closes #17093 --- src/nvim/msgpack_rpc/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/nvim/msgpack_rpc/server.c') diff --git a/src/nvim/msgpack_rpc/server.c b/src/nvim/msgpack_rpc/server.c index c9e707aa92..b252f0998e 100644 --- a/src/nvim/msgpack_rpc/server.c +++ b/src/nvim/msgpack_rpc/server.c @@ -89,7 +89,7 @@ void server_teardown(void) /// /// Named pipe format: /// - Windows: "\\.\pipe\.." -/// - Other: "~/.local/state/nvim/.." +/// - Other: "/tmp/nvim.user/xxx/.." char *server_address_new(const char *name) { static uint32_t count = 0; @@ -98,7 +98,7 @@ char *server_address_new(const char *name) int r = snprintf(fmt, sizeof(fmt), "\\\\.\\pipe\\%s.%" PRIu64 ".%" PRIu32, name ? name : "nvim", os_get_pid(), count++); #else - char *dir = get_xdg_home(kXDGStateHome); + char *dir = stdpaths_get_xdg_var(kXDGRuntimeDir); int r = snprintf(fmt, sizeof(fmt), "%s/%s.%" PRIu64 ".%" PRIu32, dir, name ? name : "nvim", os_get_pid(), count++); xfree(dir); -- cgit