New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] connect to socket (RPC only for the moment) #6594

Merged
merged 2 commits into from May 29, 2017

Conversation

Projects
None yet
6 participants
@bfredl
Member

bfredl commented Apr 26, 2017

Adds sockopen(mode, address, {options})

  • named pipe
  • tcp (ip/hostname)
  • rpc protocol
  • bytes protocol (done later in #6844)
  • sane error handling
  • tests and documentation

Currently connecting to another nvim instance over its named pipe works.

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 26, 2017

would rpcconnect make sense as the name? #3119

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 26, 2017

Why? We already made the trouble of deprecating rpcstart in favor for jobstart(..., {'rpc':v:true}. Similarily this should be sockopen(..., {'rpc':v:true}) and sockopen(..., {'rpc':v:false, 'on_data': callback})

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 26, 2017

Though it could be sockconnect or just connect().

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 26, 2017

I'm more interested in finding two common prefixes for all of the job/channel related stuff to live under. How about chconnect() or chopen()? (And later rename rpc* to ch*)

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 26, 2017

maybe, but I wonder if we then want something more distinct from vim 8:s ch_. (Though personally I don't see any strong need of fitting all of jobs, sockets, terminals, bytes io and rpc under one or two prefixes.)

@marvim marvim added the WIP label Apr 26, 2017

@bfredl bfredl force-pushed the bfredl:sockopen branch from 6e3795a to 33faeb1 Apr 27, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 27, 2017

Actually, I'm thinking of just implementing rpc in this PR which is almost done (codewise, need tests too). Before implementing bytes I want to do some refactor to the job output code, so it easier can be reused, and maybe some improvements like able to getting all data on EOF. Temporarily, sockopen("pipe", "/nvim/socket", {'rpc':v:true}) will work but 'rpc':v:false will not.

@bfredl bfredl force-pushed the bfredl:sockopen branch 3 times, most recently from 3edaff5 to 2110ac9 Apr 28, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 30, 2017

update: sockopen("tcp", "127.0.0.1:9999", {'rpc':v:true}) does work, but sockopen("tcp", "localhost:9999", {'rpc':v:true}) does not. Any TCP/IP expert here with any hints?

addrinfo = addr_req.addrinfo;
uv_tcp_t *tcp = &stream->uv.tcp;
uv_tcp_init(&loop->uv, tcp);

This comment has been minimized.

@oni-link

oni-link Apr 30, 2017

Contributor

Try to move this line after label tcp_retry so that localhost address works.

@bfredl bfredl force-pushed the bfredl:sockopen branch from 2110ac9 to e22587f Apr 30, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 30, 2017

@oni-link Thanks, that worked.

local server = spawn(nvim_argv)
set_session(server)
-- TODO: retry() with random ports?
local address = "127.0.0.1:88888"

This comment has been minimized.

@oni-link

oni-link Apr 30, 2017

Contributor

Port > 2^16-1?

@bfredl bfredl force-pushed the bfredl:sockopen branch from 0d38576 to 8c8a6ca Apr 30, 2017

@bfredl bfredl changed the title from [WIP] connect to socket to [RFC] connect to socket (RPC only for the moment) May 1, 2017

@bfredl bfredl force-pushed the bfredl:sockopen branch 2 times, most recently from f3936a8 to b0aa9ad May 1, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented May 1, 2017

marking RFC, but sometimes the test hang after connection failure, seems some uv handle needs close after failure? But not uv_close(uv_stream), it causes assertion failure.

@marvim marvim added RFC and removed WIP labels May 1, 2017

char *addr = NULL;
if (tcp) {
addr = xstrdup(address);
char *host_end = xstrchrnul(addr, ':');

This comment has been minimized.

@oni-link

oni-link May 1, 2017

Contributor

What about IPv6 addresses, they contain : at different positions? Perhaps search for the last ':' in an address like ::1:8888?

@oni-link

This comment has been minimized.

Contributor

oni-link commented May 1, 2017

marking RFC, but sometimes the test hang after connection failure, seems some uv handle needs close after failure? But not uv_close(uv_stream), it causes assertion failure.

The hangs (when exiting nvim) after a connection failure should be fixed with this patch:

diff --git a/src/nvim/event/socket.c b/src/nvim/event/socket.c
index e57aff938..048208cbe 100644
--- a/src/nvim/event/socket.c
+++ b/src/nvim/event/socket.c
@@ -174,6 +174,9 @@ static void connect_cb(uv_connect_t *req, int status)
 {
   int *ret_status = req->data;
   *ret_status = status;
+  if (status != 0) {
+    uv_close((uv_handle_t *)req->handle, NULL);
+  }
 }
 
 bool socket_connect(Loop *loop, Stream *stream,
@@ -207,12 +210,11 @@ bool socket_connect(Loop *loop, Stream *stream,
       goto cleanup;
     }
     addrinfo = addr_req.addrinfo;
+    uv_stream = (uv_stream_t *)&stream->uv.tcp;
 
-    uv_tcp_t *tcp = &stream->uv.tcp;
 tcp_retry:
-    uv_tcp_init(&loop->uv, tcp);
-    uv_tcp_connect(&req,  tcp, addrinfo->ai_addr, connect_cb);
-    uv_stream = (uv_stream_t *)tcp;
+  uv_tcp_init(&loop->uv, (uv_tcp_t *)uv_stream);
+  uv_tcp_connect(&req, (uv_tcp_t *)uv_stream, addrinfo->ai_addr, connect_cb);
 
   } else {
     uv_pipe_t *pipe = &stream->uv.pipe;
*host_end = NUL;
const struct addrinfo hints = { .ai_family = AF_UNSPEC,
.ai_flags = AI_NUMERICSERV };

This comment has been minimized.

@oni-link

oni-link May 1, 2017

Contributor

If we use tcp for a connection, we could also specify .ai_socktype = SOCK_STREAM to reduce the number of retries.

"tcp" then {address} should be of the form "host:port" where
the host should be an ip adderess or host name, and port the
port number. Currently only rpc sockets are supported, so {opts}
must be passed with "rpc" set to `v:true`.

This comment has been minimized.

@oni-link

oni-link May 1, 2017

Contributor

Numbers unequal to zero are also okay.

This comment has been minimized.

@jamessan

jamessan May 1, 2017

Member

Historically Vim represented booleans simply by using numbers, but more recently v:true/v:false were added to make it explicit. It's probably better to reword this like

Currently only rpc sockets are supported so {opts} must be passed with "rpc" set to |TRUE|.

Unless the code is explicitly checking for the v:true/v:false special values.

@bfredl bfredl force-pushed the bfredl:sockopen branch 2 times, most recently from 9eaafac to 7135e18 May 1, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented May 1, 2017

@oni-link @jamessan Thanks, updated to comments.

@bfredl bfredl force-pushed the bfredl:sockopen branch 2 times, most recently from 251a316 to 069497e May 25, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented May 28, 2017

Now that #6680 is merged this is more or less mergable. Any further discussion on the name sockopen() ?

@justinmk

This comment has been minimized.

Member

justinmk commented May 29, 2017

Though personally I don't see any strong need of fitting all of jobs, sockets, terminals, bytes io and rpc under one or two prefixes.)

Same reason that plan 9 treats sockets as files and has the same API for them: they aren't really that different. The differences are details which have been promoted to "important differences" by people who have internalized the unix API.

Using a common prefix helps discoverability and helps reinforce the connections for users/devs trying to connect the various moving parts.

@bfredl

This comment has been minimized.

Member

bfredl commented May 29, 2017

Same reason that plan 9 treats sockets as files and has the same API for them: they aren't really that different.

But that's a different concern and one we already and will continue to fulfill: use the same function for the same functionality on different object. Starting a job and connecting a socket is very different, but then sending rpc to them is more or less the same.

Using a common prefix helps discoverability and helps reinforce the connections for users/devs trying to connect the various moving parts.

This is a stronger argument. But I wonder if we can wait with that till the "channel unification" PR that will come (to support bytes io with sockets, shared close() function, introspection of channels, etc) and live with sockopen() or sockconnect() for now.

@justinmk

This comment has been minimized.

Member

justinmk commented May 29, 2017

Fair enough.

@bfredl

This comment has been minimized.

Member

bfredl commented May 29, 2017

Ok, I will change to sockconnect(), as I guess in the future it will be XX_connect() ...

@@ -6863,6 +6863,21 @@ sinh({expr}) *sinh()*
:echo sinh(-0.9)
< -1.026517
sockopen({mode}, {address}, {opts}) {Nvim} *sockopen()*

This comment has been minimized.

@justinmk

justinmk May 29, 2017

Member

feel free to leave out these {Nvim} markers, a note in vim_diff.txt is enough.

This comment has been minimized.

@bfredl

bfredl May 29, 2017

Member

removed. But maybe I'll wait with vim_diff.txt til next PR which will have more overview.

@bfredl bfredl force-pushed the bfredl:sockopen branch from 069497e to f65241c May 29, 2017

// write stream
wstream_init_fd(&main_loop, &channel->data.std.out, 1, 0);
}
/// Creates a loopback channel. This is used to avoid deadlocku

This comment has been minimized.

@justinmk

This comment has been minimized.

@bfredl

@bfredl bfredl force-pushed the bfredl:sockopen branch from f65241c to 5a15155 May 29, 2017

@bfredl bfredl merged commit 1b7a9bf into neovim:master May 29, 2017

1 of 3 checks passed

QuickBuild Build pr-6594 finished with status FAILED
Details
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jszakmeister jszakmeister removed the RFC label May 29, 2017

justinmk added a commit to justinmk/neovim that referenced this pull request May 30, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request May 30, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request May 31, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request May 31, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 5, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 6, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 6, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 6, 2017

loop_close: Avoid infinite loop, and log it.
Avoids a hang, and also helps diagnose issues like:

neovim#6594 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Jul 1, 2017

ci/quickbuild: XXX: disable server_requests test
Temporarily disable this test which hangs quickbuild.

References neovim#6594 5a15155

justinmk added a commit to justinmk/neovim that referenced this pull request Jul 1, 2017

ci/quickbuild: XXX: disable server_requests test
Temporarily disable this test which hangs quickbuild.

References neovim#6594 5a15155

justinmk added a commit that referenced this pull request Jul 1, 2017

ci/quickbuild: XXX: disable server_requests test (#6851)
Temporarily disable this test which hangs quickbuild.

From #6905: The hang occurs when calling nvim_set_current_line.

References #6594 5a15155

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment