[WIP/RFC] Implement server_address() #1909

equalsraf · 2015-01-29T20:02:02Z

os_serveraddress() returns a unique address for Neovim to listen
on when no address is supplied.
In Unix the behaviour is unchanged, it uses vim_tempname() to
get a path in the Neovim temp folder.
For Windows all local pipe addresses must start with .\pipe
followed by a name, the pipe name is set as nvim-PID.
Fixes pipe support in Windows

Following d52fb89. I think I've addressed all the comments, I just need to go back to Windows and test if I have not broken anything. ping @aktau @justinmk

equalsraf · 2015-01-29T20:08:56Z

Ups clearly I should have moved that somewhere in os/. Any suggestion where.

equalsraf · 2015-01-29T20:13:14Z

Windows requires an include for inttypes.h for PRIu64.

aktau · 2015-01-29T20:14:05Z

Windows requires an include for inttypes.h for PRIu64.

So does every other system, iirc.

equalsraf · 2015-01-29T20:17:29Z

So does every other system, iirc.

For some reason it was not building in windows without it. Maybe inttypes is included in os_unix.h/os_unix_defs

Just a question, would this interfere somehow with multiple nvim's running alongside each other?

Should not interfere. For Unix it does not change the current behaviour. For windows the pipes are named nvim- so that each running process gets a unique address. With two running instances I get

C:\Users\raf\Dev\neovim\build>pipelist.exe  | grep nvim
nvim-11924                                        4               -1
nvim-1964

aktau · 2015-01-29T20:19:53Z

For some reason it was not building in windows without it. Maybe inttypes is included in os_unix.h/os_unix_defs

Likely. This is an ongoing problem :).

Should not interfere.

Yea, I forgot that the environment variable would only be set for the terminal from which nvim was run.

justinmk · 2015-02-04T22:04:58Z

Seems like this has overlap with #1758. Can we add a api/vim.c:vim_get_instance_id() in this PR that just returns os_serveraddress()?

equalsraf · 2015-02-04T23:44:10Z

I can, but the random properties of one and the other are not the same:

In Windows this is solely dependent on pid
Unix seems dependent on libuv's implementation of mkdtemp

i.e. there can be no two instances of Neovim running at the same time where os_serveraddress() is the same, but it would be possible for two instances that run sequentially to have the same serveraddress - with what probability I cannot say.

The goal of #1758 was different, it was looking for something along the lines of an UUID - completely unique over time.

Also plugins can already get the value as returned by os_serveraddress, by requesting $NVIM_LISTEN_ADDRESS (e.g. eval via the api).

justinmk · 2015-02-05T00:28:26Z

Ok, good points.

phmarek · 2015-02-05T09:29:55Z

One more comment: vim_get_instance_id() also works if there are multiple instances visible on one machine. Eg via port forwarding a local plugin might connect to two Neovim instances on different machines that have both the same pid and/or pipe and/or temporary directory etc.

justinmk · 2015-05-21T01:44:25Z

src/nvim/msgpack_rpc/server.c

+///
+const char* os_serveraddress(void)
+{
+  static char servername[ADDRESS_MAX_SIZE] = "";


What happens if serverstop() is called on this address?

justinmk · 2015-05-21T01:45:58Z

Need a "tickle test" for unix and windows to verify that the correct format is returned. Also need a test to assert behavior after call serverstop("...") is called on the address.

tarruda · 2015-09-30T13:21:01Z

@equalsraf is this still an issue after @splinterofchaos implemented serverstart()/serverstop()?

equalsraf · 2015-09-30T13:39:45Z

@tarruda yes it is.

a default server name is still being generated when serverstart takes no arguments in Windows needs to be a valid pipe name
server_init() is still being used - but can probably be replaced with serverstart($NVIM_LISTEN_ADDRESS)?

tarruda · 2015-09-30T13:43:42Z

@equalsraf ok, can rebase these changes on latest master?

justinmk · 2015-09-30T14:26:59Z

Still have questions in my comments above.

equalsraf · 2015-09-30T14:41:28Z

@justinmk not forgotten (:D at least not completely) need to fix it anyway for serverstart and it should address it too.

equalsraf · 2015-09-30T23:55:36Z

Rewrote most of it, renamed as server_name() - it no longer returns a unique address, just mimics vim_tempname().

In UNIX just calls vim_tempname(). For Windows it returns \.\pipe\nvim-PID-COUNTER where COUNTER is a static counter in server_name().

As before the PID ensures there are no collisions with other Neovim instaces, the counter increments with each call - I have not checked for a wrap around, but neither does vim_tempname(), in both cases it should be harmless and result in a failed call to server_start.

Changed server_start() and f_serverstart () to use this instead of vim_tempname().

@justinmk for windows serverlist() gives me ['\.\pipe\nvim-6780-0', '\.\pipe\nvim-6780-1']. serverstop('\\.\pipe\nvim-6780-1') closes the server as expected. serverstart('xxxxxxx') fails with permission denied.

oni-link · 2015-10-01T21:17:00Z

src/nvim/msgpack_rpc/server.c

+#ifdef WIN32
+  static uint32_t count = 0;
+  char template[ADDRESS_MAX_SIZE] = "";
+  if (template[0] == '\0') {


Why use an if? The condition is always true.

Leftover, template used to be a static buffer with the value being initialized only once. Removing the if and the initializer.

tarruda · 2015-10-02T13:53:55Z

Since windows will not be supported by 0.1, postponing it

justinmk · 2015-10-17T18:39:22Z

src/nvim/msgpack_rpc/server.c

+/// For other systems it is a path returned by vim_tempname().
+///
+/// This function is NOT thread safe
+char *server_address(void)


server_address_create or server_address_generate would be a less ambiguous name.

Would server_address_new() be better?

justinmk · 2015-10-17T18:44:20Z

@equalsraf Could you address (minor) comments and rebase this? It is needed for XDG.

When creating a local socket/pipe (server_start()) Neovim used vim_tempname() to generate a unique socket path. For Windows UNIX filepaths cannot be used as pipe names (they must start with \\.\pipe\). This commit replaces the use of vim_tempname() for server addresses with server_address_new(). server_address_new() generates unique names for local sockets/pipes - for UNIX it uses vim_tempname(), for Windows generates names in the form \\.\pipe\nvim-PID-COUNTER where PID is the current process id, and COUNTER is a static uint32_t counter incremented with every call. This function is now used for server_start() and server_init() when no address is available.

Return 1 if the endpoint argument is NULL, server_start() can get a NULL value when using server_address_new() or vim_tempname(). Removed the function attribute.

equalsraf · 2015-10-18T19:21:15Z

I've rebased this yesterday and again today, but Travis seems to be stuck.

justinmk · 2015-10-18T19:39:57Z

@equalsraf Thanks. I emailed travis today, waiting for reply.

server_address()

equalsraf force-pushed the tb-os_serveraddress branch from bd82441 to ed59b98 Compare January 29, 2015 20:06

marvim added the RFC label Jan 29, 2015

equalsraf force-pushed the tb-os_serveraddress branch from ed59b98 to 96bbda3 Compare January 29, 2015 20:26

equalsraf force-pushed the tb-os_serveraddress branch from 96bbda3 to 7b313bd Compare April 17, 2015 10:15

equalsraf mentioned this pull request May 20, 2015

[WIP] Compiling under Windows #810

Closed

equalsraf force-pushed the tb-os_serveraddress branch from 7b313bd to fe64ad7 Compare May 20, 2015 22:45

justinmk reviewed May 21, 2015
View reviewed changes

justinmk added this to the 0.1-first-public-release milestone Aug 24, 2015

equalsraf changed the title ~~[RFC] Implement os_serveraddress()~~ [WIP/RFC] Implement os_serveraddress() Sep 30, 2015

equalsraf force-pushed the tb-os_serveraddress branch 4 times, most recently from f995327 to 433a89e Compare September 30, 2015 16:24

equalsraf changed the title ~~[WIP/RFC] Implement os_serveraddress()~~ [WIP/RFC] Implement server_address() Sep 30, 2015

tarruda self-assigned this Oct 1, 2015

oni-link reviewed Oct 1, 2015
View reviewed changes

equalsraf force-pushed the tb-os_serveraddress branch from 433a89e to 07fb5c0 Compare October 1, 2015 21:55

tarruda modified the milestones: 0.2, 0.1-first-public-release Oct 2, 2015

ZyX-I mentioned this pull request Oct 17, 2015

XDG base directory specification support #3470

Merged

justinmk reviewed Oct 17, 2015
View reviewed changes

equalsraf force-pushed the tb-os_serveraddress branch from f5da242 to 3dc8b0f Compare October 17, 2015 20:54

equalsraf added 2 commits October 18, 2015 16:34

Allow server_start to accept a NULL argument

3e84a91

Return 1 if the endpoint argument is NULL, server_start() can get a NULL value when using server_address_new() or vim_tempname(). Removed the function attribute.

equalsraf force-pushed the tb-os_serveraddress branch from 3dc8b0f to 3e84a91 Compare October 18, 2015 15:35

justinmk added a commit that referenced this pull request Oct 19, 2015

Merge pull request #1909 from equalsraf/tb-os_serveraddress

e38cbb9

server_address()

justinmk merged commit e38cbb9 into neovim:master Oct 19, 2015

justinmk removed the RFC label Oct 19, 2015

equalsraf deleted the tb-os_serveraddress branch February 22, 2016 22:30

Uh oh!

[WIP/RFC] Implement server_address() #1909

[WIP/RFC] Implement server_address() #1909

Uh oh!

Conversation

equalsraf commented Jan 29, 2015

Uh oh!

equalsraf commented Jan 29, 2015

Uh oh!

equalsraf commented Jan 29, 2015

Uh oh!

aktau commented Jan 29, 2015

Uh oh!

equalsraf commented Jan 29, 2015

Uh oh!

aktau commented Jan 29, 2015

Uh oh!

justinmk commented Feb 4, 2015

Uh oh!

equalsraf commented Feb 4, 2015

Uh oh!

justinmk commented Feb 5, 2015

Uh oh!

phmarek commented Feb 5, 2015

Uh oh!

justinmk May 21, 2015

Choose a reason for hiding this comment

Uh oh!

justinmk commented May 21, 2015

Uh oh!

tarruda commented Sep 30, 2015

Uh oh!

equalsraf commented Sep 30, 2015

Uh oh!

tarruda commented Sep 30, 2015

Uh oh!

justinmk commented Sep 30, 2015

Uh oh!

equalsraf commented Sep 30, 2015

Uh oh!

equalsraf commented Sep 30, 2015

Uh oh!

oni-link Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

equalsraf Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

tarruda commented Oct 2, 2015

Uh oh!

justinmk Oct 17, 2015

Choose a reason for hiding this comment

Uh oh!

equalsraf Oct 17, 2015

Choose a reason for hiding this comment

Uh oh!

justinmk commented Oct 17, 2015

Uh oh!

equalsraf commented Oct 18, 2015

Uh oh!

justinmk commented Oct 18, 2015

Uh oh!

Uh oh!