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?

runtime/doc/builtin.txt

@@ -6628,30 +6628,29 @@ serverlist()						*serverlist()*
 
 serverstart([{address}])				*serverstart()*
 		Opens a socket or named pipe at {address} and listens for
-		|RPC| messages. Clients can send |API| commands to the address
+		|RPC| messages. Clients can send |API| commands to the
-		to control Nvim.
+		returned address to control Nvim.
 
-		Returns the address string.
+		Returns the address string (may differ from the requested
+		{address}).
+		 
+		- If {address} contains a colon ":" it is interpreted as
+		  a TCP/IPv4/IPv6 address where the last ":" separates host
+		  and port (empty or zero assigns a random port).
+		- Else it is interpreted as a named pipe or Unix domain socket
+		  path. If there are no slashes it is treated as a name and
+		  appended to a generated path.
+		- If {address} is empty it generates a path.
 
-		If {address} does not contain a colon ":" it is interpreted as
+		Example named pipe: >
-		a named pipe or Unix domain socket path.
-
-		Example: >
 			if has('win32')
-			  call serverstart('\\.\pipe\nvim-pipe-1234')
+			  echo serverstart('\\.\pipe\nvim-pipe-1234')
 			else
-			  call serverstart('nvim.sock')
+			  echo serverstart('nvim.sock')
 			endif
 <
-		If {address} contains a colon ":" it is interpreted as a TCP
+		Example TCP/IP address: >
-		address where the last ":" separates the host and port.
+			echo serverstart('::1:12345')
-		Assigns a random port if it is empty or 0. Supports IPv4/IPv6.
-
-		Example: >
-			:call serverstart('::1:12345')
-<
-		If no address is given, it is equivalent to: >
-			:call serverstart(tempname())
 
 serverstop({address})					*serverstop()*
 		Closes the pipe or socket at {address}.
@@ -7545,7 +7544,7 @@ stdpath({what})					*stdpath()* *E6100*
 		data_dirs    List    Other data directories.
 		log          String  Logs directory (for use by plugins too).
 		state        String  Session state directory: storage for file
-				     drafts, undo history, shada, etc.
+				     drafts, undo, shada, named pipes, ...
 
 		Example: >
 			:echo stdpath("config")

src/nvim/eval/funcs.c

@@ -8497,7 +8497,7 @@ static void f_serverstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)
       address = xstrdup(tv_get_string(argvars));
     }
   } else {
-    address = server_address_new();
+    address = server_address_new(NULL);
   }
 
   int result = server_start(address);

src/nvim/log.c

@@ -16,11 +16,13 @@
 #include <uv.h>
 
 #include "auto/config.h"
+#include "nvim/eval.h"
 #include "nvim/log.h"
 #include "nvim/main.h"
 #include "nvim/message.h"
 #include "nvim/os/os.h"
 #include "nvim/os/time.h"
+#include "nvim/path.h"
 #include "nvim/types.h"
 
 /// Cached location of the expanded log file path decided by log_path_init().
@@ -291,8 +293,7 @@ static bool v_do_log_to_file(FILE *log_file, int log_level, const char *context,
     return false;
   }
   char date_time[20];
-  if (strftime(date_time, sizeof(date_time), "%Y-%m-%dT%H:%M:%S",
+  if (strftime(date_time, sizeof(date_time), "%Y-%m-%dT%H:%M:%S", &local_time) == 0) {
-               &local_time) == 0) {
     return false;
   }
 
@@ -303,14 +304,19 @@ static bool v_do_log_to_file(FILE *log_file, int log_level, const char *context,
   }
 
   // Get a name for this Nvim instance.
-  if (name[0] == '\0') {
+  // TODO(justinmk): expose this as v:name ?
-    const char *testid = os_getenv("NVIM_TEST");
+  if (starting || name[0] == '\0') {
-    const char *parent = os_getenv(ENV_NVIM);
+    // Parent servername.
-    if (testid) {
+    const char *parent = path_tail(os_getenv(ENV_NVIM));
-      snprintf(name, sizeof(name), "%s%s", testid ? testid : "", parent ? "/child" : "");
+    // Servername. Empty until starting=false.
+    const char *serv = path_tail(get_vim_var_str(VV_SEND_SERVER));
+    if (parent && parent[0] != NUL) {
+      snprintf(name, sizeof(name), "%s/c", parent);  // "/c" indicates child.
+    } else if (serv && serv[0] != NUL) {
+      snprintf(name, sizeof(name), "%s", serv ? serv : "");
     } else {
       int64_t pid = os_get_pid();
-      snprintf(name, sizeof(name), "%-5" PRId64 "%s", pid, parent ? "/child" : "");
+      snprintf(name, sizeof(name), "?.%-5" PRId64, pid);
     }
   }
 

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
+/// Named pipe format:
-///     \\.\pipe\nvim-<PID>-<COUNTER>.
+/// - Windows: "\\.\pipe\<name>.<pid>.<counter>"
-///
+/// - Other: "~/.local/state/nvim/<name>.<pid>.<counter>"
-/// For other systems it is a path returned by vim_tempname().
+char *server_address_new(const char *name)
-///
-/// This function is NOT thread safe
-char *server_address_new(void)
 {
-#ifdef WIN32
   static uint32_t count = 0;
-  char template[ADDRESS_MAX_SIZE];
+  char fmt[ADDRESS_MAX_SIZE];
-  snprintf(template, ADDRESS_MAX_SIZE,
+#ifdef WIN32
-           "\\\\.\\pipe\\nvim-%" PRIu64 "-%" PRIu32, os_get_pid(), count++);
+  int r = snprintf(fmt, sizeof(fmt), "\\\\.\\pipe\\%s.%" PRIu64 ".%" PRIu32,
-  return xstrdup(template);
+                   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.
+/// Starts listening for RPC calls.
 ///
-/// The socket type is determined by parsing `endpoint`: If it's a valid IPv4
+/// Socket type is decided by the format of `addr`:
-/// or IPv6 address in 'ip:[port]' format, then it will be a TCP socket.
+/// - TCP socket if it looks like an IPv4/6 address ("ip:[port]").
-/// Otherwise it will be a Unix socket or named pipe (Windows).
+///   - 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
 ///
-/// If no port is given, a random one will be assigned.
+/// @param addr Server address: a "ip:[port]" string or arbitrary name or filepath (max 256 bytes)
-///
+///             for the Unix socket or named pipe.
-/// @param endpoint Address of the server. Either a 'ip:[port]' string or an
+/// @returns 0: success, 1: validation error, 2: already listening, -errno: failed to bind/listen.
-///                 arbitrary identifier (trimmed to 256 bytes) for the Unix
+int server_start(const char *addr)
-///                 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)
 {
-  if (endpoint == NULL || endpoint[0] == '\0') {
+  if (addr == NULL || addr[0] == '\0') {
-    WLOG("Empty or NULL endpoint");
+    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, isname ? addr_gen : addr);
-  int result = socket_watcher_init(&main_loop, watcher, endpoint);
+  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);

src/nvim/path.c

@@ -88,7 +88,12 @@ FileComparison path_full_compare(char_u *const s1, char_u *const s2, const bool
   return kDifferentFiles;
 }
 
-/// Gets the tail (i.e., the filename segment) of a path `fname`.
+/// Gets the tail (filename segment) of path `fname`.
+///
+/// Examples:
+/// - "dir/file.txt" => "file.txt"
+/// - "file.txt" => "file.txt"
+/// - "dir/" => ""
 ///
 /// @return pointer just past the last path separator (empty string, if fname
 ///         ends in a slash), or empty string if fname is NULL.

test/README.md

@@ -91,25 +91,33 @@ or:
 Debugging tests
 ---------------
 
+- Each test gets a test id which looks like "T123". This also appears in the
+  log file. Child processes spawned from a test appear in the logs with the
+  *parent* name followed by "/c". Example:
+  ```
+    DBG 2022-06-15T18:37:45.226 T57.58016.0   UI: flush
+    DBG 2022-06-15T18:37:45.226 T57.58016.0   inbuf_poll:442: blocking... events_enabled=0 events_pending=0
+    DBG 2022-06-15T18:37:45.227 T57.58016.0/c UI: stop
+    INF 2022-06-15T18:37:45.227 T57.58016.0/c os_exit:595: Nvim exit: 0
+    DBG 2022-06-15T18:37:45.229 T57.58016.0   read_cb:118: closing Stream (0x7fd5d700ea18): EOF (end of file)
+    INF 2022-06-15T18:37:45.229 T57.58016.0   on_process_exit:400: exited: pid=58017 status=0 stoptime=0
+  ```
 - You can set `$GDB` to [run tests under gdbserver](https://github.com/neovim/neovim/pull/1527).
   And if `$VALGRIND` is set it will pass `--vgdb=yes` to valgrind instead of
   starting gdbserver directly.
-- Hanging tests often happen due to unexpected `:h press-enter` prompts. The
+- Hanging tests can happen due to unexpected "press-enter" prompts. The
   default screen width is 50 columns. Commands that try to print lines longer
   than 50 columns in the command-line, e.g. `:edit very...long...path`, will
-  trigger the prompt. In this case, a shorter path or `:silent edit` should be
+  trigger the prompt. Try using a shorter path, or `:silent edit`.
-  used.
 - If you can't figure out what is going on, try to visualize the screen. Put
   this at the beginning of your test:
-
+  ```lua
-    ```lua
+  local Screen = require('test.functional.ui.screen')
-    local Screen = require('test.functional.ui.screen')
+  local screen = Screen.new()
-    local screen = Screen.new()
+  screen:attach()
-    screen:attach()
+  ```
-    ```
+  Then put `screen:snapshot_util()` anywhere in your test. See the comments in
-
+  `test/functional/ui/screen.lua` for more info.
-  Afterwards, put `screen:snapshot_util()` at any position in your test. See the
-  comment at the top of `test/functional/ui/screen.lua` for more.
 
 Filtering Tests
 ---------------
@@ -276,9 +284,6 @@ Number; !must be defined to function properly):
 
 - `NVIM_PRG` (F) (S): path to Nvim executable (default: `build/bin/nvim`).
 
-- `NVIM_TEST` (F) (S): Test id (example: "T1000") generated by the test runner
-  and prepended to `$NVIM_LOG_FILE` messages.
-
 - `NVIM_TEST_MAIN_CDEFS` (U) (1): makes `ffi.cdef` run in main process. This
   raises a possibility of bugs due to conflicts in header definitions, despite
   the counters, but greatly speeds up unit tests by not requiring `ffi.cdef` to

test/functional/core/log_spec.lua

@@ -1,18 +1,19 @@
 local helpers = require('test.functional.helpers')(after_each)
 local assert_log = helpers.assert_log
 local clear = helpers.clear
+local command = helpers.command
 local eq = helpers.eq
 local exec_lua = helpers.exec_lua
+local expect_exit = helpers.expect_exit
 local request = helpers.request
 local retry = helpers.retry
-local expect_exit = helpers.expect_exit
 
 describe('log', function()
-  local test_log_file = 'Xtest_logging'
+  local testlog = 'Xtest_logging'
 
   after_each(function()
-    expect_exit('qa!')
+    expect_exit(command, 'qa!')
-    os.remove(test_log_file)
+    os.remove(testlog)
   end)
 
   it('skipped before log_init', function()
@@ -33,13 +34,14 @@ describe('log', function()
     --    ERR 2022-05-29T12:30:03.814 T2/child   log_init:110: test log message
 
     clear({env={
-      NVIM_LOG_FILE=test_log_file,
+      NVIM_LOG_FILE=testlog,
-      -- TODO: Can remove this after nvim_log #7062 is merged.
+      -- TODO: remove this after nvim_log #7062 is merged.
       __NVIM_TEST_LOG='1'
       }})
 
-    retry(nil, nil, function()
+    local tid = _G._nvim_test_id
-      assert_log('T%d+\\.%d+\\.\\d +log_init:%d+: test log message', test_log_file, 100)
+    retry(nil, 1000, function()
+      assert_log(tid..'%.%d+%.%d +server_init:%d+: test log message', testlog, 100)
     end)
 
     exec_lua([[
@@ -47,9 +49,9 @@ describe('log', function()
       vim.fn.jobwait({ j1 }, 10000)
     ]])
 
-    -- Child Nvim spawned by jobstart() appends "/child" to parent name.
+    -- Child Nvim spawned by jobstart() appends "/c" to parent name.
-    retry(nil, nil, function()
+    retry(nil, 1000, function()
-      assert_log('T%d+/child +log_init:%d+: test log message', test_log_file, 100)
+      assert_log('%.%d+%.%d/c +server_init:%d+: test log message', testlog, 100)
     end)
   end)
 end)

test/functional/helpers.lua

@@ -431,16 +431,20 @@ end
 function module.new_argv(...)
   local args = {unpack(module.nvim_argv)}
   table.insert(args, '--headless')
+  if _G._nvim_test_id then
+    -- Set the server name to the test-id for logging. #8519
+    table.insert(args, '--listen')
+    table.insert(args, _G._nvim_test_id)
+  end
   local new_args
   local io_extra
-  local env = {}
+  local env = nil
   local opts = select(1, ...)
   if type(opts) ~= 'table' then
     new_args = {...}
   else
     args = remove_args(args, opts.args_rm)
     if opts.env then
-      opts.env['NVIM_TEST'] = nil
       local env_opt = {}
       for k, v in pairs(opts.env) do
         assert(type(k) == 'string')
@@ -461,11 +465,12 @@ function module.new_argv(...)
         'TMPDIR',
         'VIMRUNTIME',
       }) do
-        -- Set these from the environment only if not in opts.env.
+        -- Set these from the environment unless the caller defined them.
         if not env_opt[k] then
           env_opt[k] = os.getenv(k)
         end
       end
+      env = {}
       for k, v in pairs(env_opt) do
         env[#env + 1] = k .. '=' .. v
       end
@@ -476,10 +481,6 @@ function module.new_argv(...)
   for _, arg in ipairs(new_args) do
     table.insert(args, arg)
   end
-
-  -- TODO(justinmk): introduce v:name and use that instead.
-  table.insert(env, ('NVIM_TEST=%s'):format(_G._nvim_test_id or '?'))
-
   return args, env, io_extra
 end
 

test/functional/terminal/api_spec.lua

@@ -6,7 +6,7 @@ if helpers.pending_win32(pending) then return end
 
 describe('api', function()
   local screen
-  local socket_name = "Xtest_functional_api.sock"
+  local socket_name = "./Xtest_functional_api.sock"
 
   before_each(function()
     helpers.clear()
@@ -29,7 +29,7 @@ describe('api', function()
       {4:~                                                 }|
       {4:~                                                 }|
       {4:~                                                 }|
-      ]]..socket_name..[[                         |
+      ]]..socket_name..[[                       |
       {3:-- TERMINAL --}                                    |
     ]])
 

test/functional/vimscript/server_spec.lua

@@ -30,7 +30,7 @@ describe('server', function()
     eq('', eval('$NVIM_LISTEN_ADDRESS'))
     local servers = funcs.serverlist()
     eq(1, #servers)
-    ok(string.len(servers[1]) > 4)  -- Like /tmp/nvim…/… or \\.\pipe\…
+    ok(string.len(servers[1]) > 4)  -- "~/.local/state/nvim…/…" or "\\.\pipe\…"
   end)
 
   it('sets v:servername at startup or if all servers were stopped', function()
@@ -54,7 +54,7 @@ describe('server', function()
 
     -- v:servername and $NVIM take the next available server.
     local servername = (iswin() and [[\\.\pipe\Xtest-functional-server-pipe]]
-                                or 'Xtest-functional-server-socket')
+                                or './Xtest-functional-server-socket')
     funcs.serverstart(servername)
     eq(servername, meths.get_vvar('servername'))
     -- Not set in the current process, only in children.
@@ -66,7 +66,7 @@ describe('server', function()
     eq(0, eval("serverstop('bogus-socket-name')"))
   end)
 
-  it('parses endpoints correctly', function()
+  it('parses endpoints', function()
     clear_serverlist()
     eq({}, funcs.serverlist())
 
@@ -101,6 +101,10 @@ describe('server', function()
     eq(expected, funcs.serverlist())
     clear_serverlist()
 
+    -- Address without slashes is a "name" which is appended to a generated path. #8519
+    matches([[.*[/\\]xtest1%.2%.3%.4[^/\\]*]], funcs.serverstart('xtest1.2.3.4'))
+    clear_serverlist()
+
     eq('Vim:Failed to start server: invalid argument',
       pcall_err(funcs.serverstart, '127.0.0.1:65536'))  -- invalid port
     eq({}, funcs.serverlist())
@@ -113,7 +117,7 @@ describe('server', function()
     -- Add some servers.
     local servs = (iswin()
       and { [[\\.\pipe\Xtest-pipe0934]], [[\\.\pipe\Xtest-pipe4324]] }
-      or  { [[Xtest-pipe0934]], [[Xtest-pipe4324]] })
+      or  { [[./Xtest-pipe0934]], [[./Xtest-pipe4324]] })
     for _, s in ipairs(servs) do
       eq(s, eval("serverstart('"..s.."')"))
     end
@@ -146,9 +150,13 @@ describe('startup --listen', function()
 
   it('sets v:servername, overrides $NVIM_LISTEN_ADDRESS', function()
     local addr = (iswin() and [[\\.\pipe\Xtest-listen-pipe]]
-                          or 'Xtest-listen-pipe')
+                          or './Xtest-listen-pipe')
-    clear({ env={ NVIM_LISTEN_ADDRESS='Xtest-env-pipe' },
+    clear({ env={ NVIM_LISTEN_ADDRESS='./Xtest-env-pipe' },
             args={ '--listen', addr } })
     eq(addr, meths.get_vvar('servername'))
+
+    -- Address without slashes is a "name" which is appended to a generated path. #8519
+    clear({ args={ '--listen', 'test-name' } })
+    matches([[.*[/\\]test%-name[^/\\]*]], meths.get_vvar('servername'))
   end)
 end)