[RFC] don't delay notifications when request is pending #6544

bfredl · 2017-04-18T10:39:46Z

Broken out of #6532.

This logic would be much better on the client side, so it can decide when it is ready to process what notifications. If an rplugin wants to avoid async calls while a sync call is busy, it likely wants to avoid processing async calls while another async call also is handled as well. Also if a gui makes a blocking request that requires user interaction (like funcs.input()) it will currently not get any screen updates...

This will likely break the expectation of some existing rplugins. Hosts should be updated, they could even add a flag which async methods that are always safe to call (like the handler of #6532 will be) and which must wait until the main loop is returned.

justinmk · 2017-04-18T11:29:01Z

Makes sense, but what happens if a client is single-threaded? Will channel_write() hang?

Let's wait until after 0.2 since this has some risk.

bfredl · 2017-04-18T11:38:43Z

pending_requests is not a good indicator of "is the client currently unable to read from the pipe". On the contrary, when it is nonzero the situation is most likely that the client is waiting for the reply from a nested request, and thus explicitly waiting for data on the pipe...

A well designed client will be prepared to always read from the pipe, even if waiting for an event from some other source, and should off-load long-running CPU processing on a thread. But that's a concern unrelated to this PR.

justinmk · 2017-04-18T11:44:19Z

Should we show some error if channel_write takes longer than N ms?

bfredl · 2017-04-18T12:27:19Z

Actually, doesn't libuv/wstream handle the buffering (in case pipe is blocked) already?

justinmk · 2017-04-18T13:37:43Z

Maybe, I didn't check.

bfredl · 2017-06-17T09:20:37Z

Update for python-client: neovim/pynvim#262. Note this is only for backwards-compatibility with the many existing python plugins. Newer hosts could just as well expose the full asyncness to new plugins.

justinmk · 2017-06-17T19:31:59Z

can pending_requests be removed now?
explanation should be in the commit message
should this be called out in msgpack_rpc.txt and/or api.txt ?

bfredl · 2017-06-17T20:26:11Z

can pending_requests be removed now?

Probably, will check tomorrow.

should this be called out in msgpack_rpc.txt and/or api.txt ?

This change only makes nvim behave as documented (help rcpnotify()). It does not need to be "called out" to new consumers of the rpc api, who would presumably assume async to mean async. It was the old behavior that was ad-hoc and inconsistent and should have been documented. Rather it should be mentioned in release notes and similar, for people accustomed to the old behavior.

justinmk · 2017-06-17T20:43:23Z

:help rpcnotify doesn't mention that notifications may occur between request and response. So my suggestion was to mention this explicitly, just like in :help rpc-remote-ui we explicitly call out "Future versions of Nvim may add new update kinds", even though one might consider that obvious.

justinmk · 2017-06-17T20:51:32Z

d83868f explains why the delay logic was added. Need to analyze whether it is still relevant.

bfredl · 2017-06-17T21:05:27Z

You mean between request and response in one direction but not the other? rpcnotify() states that notification will be sent and function returns immediately.

bfredl · 2017-06-17T21:09:28Z

That commit reads "a client was broken. Instead of fixing it, add arbitrary restriction to all clients." (That doesn't solve the problem, an non-synced client could break the call stack as well from external source event)

justinmk · 2017-06-17T21:17:49Z

The test will need to be deleted then.

What specifically is our advice to clients? Is it "never send a request until the response for a previous request was received"?

bfredl · 2017-06-17T21:27:34Z

Why? A refactor of RPC logic could break synchronization in other ways, more tests of nested requests/notifications does not hurt.

Yes, unless there is another request from nvim between them, linking the call stack.

bfredl · 2017-06-17T21:29:07Z

Sorry I assumed you meant my test. I will look into that one tomorrow.

bfredl · 2017-10-24T12:40:01Z

@justinmk I managed to make it completely backwards-compatible for rplugins, which can opt-in to the new behavior, otherwise they will receive events in exactly the same order as before. However, UIs always get the updates immediately, which is what I think any UI developer would expect.

So I actually think it should be safe to ship this in 0.2.1. But you decide.

bfredl · 2017-10-27T18:18:35Z

It seems that that busted on travis (with the latest iteration, mostly only on OS X) simply crashes sometimes when nvim decides to disconnect. IIRC we have no other test that passes with nvim disconnecting, only tests that checks that and passes when nvim is still alive and connected.

@justinmk We could mark this test as pending for now so we can merge this for 0.2.1. The "positive" tests that checks that it actually works has long passed without problem (even on quickbuild before, which seems to have given up the ghost)

For compatibily, rplugin infrastructure reimplements this behavior by default, it can be turned off using sync="urgent".

justinmk · 2017-10-29T01:19:47Z

src/nvim/msgpack_rpc/channel.c

@@ -704,11 +689,7 @@ static void broadcast_event(const char *name, Array args)

  for (size_t i = 0; i < kv_size(subscribed); i++) {
    Channel *channel = kv_A(subscribed, i);
-    if (channel->pending_requests) {
-      kv_push(channel->delayed_notifications, buffer);


unrelated to this PR, is wstream_release_wbuffer(buffer) needed?

No, channel_write does it.

justinmk · 2017-10-29T01:33:46Z

runtime/autoload/remote/define.vim

 endfunction

+function! remote#define#notify(chan, ...)


can this and remote#define#request be script-local? s:notify, s:request

blah, it will require a bunch of changes to callers of s:GetRpcFunction().

It is called by functions outside of this file (even though they are "generated" inside it), so even if it is possible, "sematically" it is wrong (until vimL gets hygenic macros).

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

…m#6544)" This reverts commit 2a3bcd1.

marvim added the RFC label Apr 18, 2017

justinmk added this to the 0.2.1 milestone Apr 18, 2017

bfredl mentioned this pull request Jun 17, 2017

rplugin: conditionally enable nested notifications neovim/pynvim#262

Merged

bfredl force-pushed the nodelay branch 2 times, most recently from dcb4a0b to b9fb15c Compare June 17, 2017 10:04

justinmk modified the milestones: 0.2.1, 0.2.2 Sep 16, 2017

justinmk approved these changes Oct 24, 2017

View reviewed changes

bfredl force-pushed the nodelay branch 4 times, most recently from 87e014f to ea304fd Compare October 24, 2017 12:35

bfredl force-pushed the nodelay branch 2 times, most recently from 8554517 to fca8fcc Compare October 24, 2017 20:37

bfredl force-pushed the nodelay branch 7 times, most recently from 1c84427 to a2ba9c7 Compare October 27, 2017 18:11

bfredl force-pushed the nodelay branch 4 times, most recently from ca64ddf to aba9ab2 Compare October 28, 2017 19:10

msgpack_rpc: Don't delay notifications when request is pending

45a5eb0

For compatibily, rplugin infrastructure reimplements this behavior by default, it can be turned off using sync="urgent".

bfredl force-pushed the nodelay branch from aba9ab2 to 45a5eb0 Compare October 28, 2017 20:09

justinmk reviewed Oct 29, 2017

View reviewed changes

justinmk merged commit 2a3bcd1 into neovim:master Oct 29, 2017

justinmk mentioned this pull request Nov 7, 2017

test/ci: ruby_spec.lua failure #7504

Closed

justinmk mentioned this pull request Nov 8, 2017

NVIM v0.2.1 #7505

Merged

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

Revert "rpc: Don't delay notifications when request is pending (neovi…

ad7e990

…m#6544)" This reverts commit 2a3bcd1.

justinmk mentioned this pull request Nov 11, 2017

Revert "rpc: Don't delay notifications when request is pending (#6544)" #7540

Closed

justinmk mentioned this pull request Jun 26, 2018

API: nvim_buf_attach not returning #8634

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] don't delay notifications when request is pending #6544

[RFC] don't delay notifications when request is pending #6544

bfredl commented Apr 18, 2017

justinmk commented Apr 18, 2017 •

edited

bfredl commented Apr 18, 2017

justinmk commented Apr 18, 2017

bfredl commented Apr 18, 2017

justinmk commented Apr 18, 2017

bfredl commented Jun 17, 2017

justinmk commented Jun 17, 2017

bfredl commented Jun 17, 2017

justinmk commented Jun 17, 2017

justinmk commented Jun 17, 2017 •

edited

bfredl commented Jun 17, 2017

bfredl commented Jun 17, 2017

justinmk commented Jun 17, 2017

bfredl commented Jun 17, 2017

bfredl commented Jun 17, 2017

bfredl commented Oct 24, 2017

bfredl commented Oct 27, 2017 •

edited

justinmk Oct 29, 2017 •

edited

justinmk Oct 29, 2017 •

edited

bfredl Oct 29, 2017

[RFC] don't delay notifications when request is pending #6544

[RFC] don't delay notifications when request is pending #6544

Conversation

bfredl commented Apr 18, 2017

justinmk commented Apr 18, 2017 • edited

bfredl commented Apr 18, 2017

justinmk commented Apr 18, 2017

bfredl commented Apr 18, 2017

justinmk commented Apr 18, 2017

bfredl commented Jun 17, 2017

justinmk commented Jun 17, 2017

bfredl commented Jun 17, 2017

justinmk commented Jun 17, 2017

justinmk commented Jun 17, 2017 • edited

bfredl commented Jun 17, 2017

bfredl commented Jun 17, 2017

justinmk commented Jun 17, 2017

bfredl commented Jun 17, 2017

bfredl commented Jun 17, 2017

bfredl commented Oct 24, 2017

bfredl commented Oct 27, 2017 • edited

justinmk Oct 29, 2017 • edited

Choose a reason for hiding this comment

justinmk Oct 29, 2017 • edited

Choose a reason for hiding this comment

bfredl Oct 29, 2017

Choose a reason for hiding this comment

justinmk commented Apr 18, 2017 •

edited

justinmk commented Jun 17, 2017 •

edited

bfredl commented Oct 27, 2017 •

edited

justinmk Oct 29, 2017 •

edited

justinmk Oct 29, 2017 •

edited