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] channels: support buffered output and bytes sockets/stdio #6844

Merged
merged 17 commits into from Nov 26, 2017

Conversation

Projects
None yet
9 participants
@bfredl
Member

bfredl commented Jun 4, 2017

Adds the following:

  • support bytes protocol also in socket and stdio channels
  • signal EOF in output handler, and add a mode where callback gets all output at once (with buffer limit?)
  • introspection of all channels and jobs (do it in #6743)
@bfredl

This comment has been minimized.

Member

bfredl commented Jun 4, 2017

Also change I would make, but in a separate PR: move almost all job/channel code from eval.c to channel.c, also expose functionality natively in lua, so lua functions/callbacks can use lua binary strings instead of linelists, also rpc functions can do directly lua<->msgpack (primarily for semantics/convenience, only secondarily for performance).

lua callbacks could even be run more asynchronously than vimL callbacks (i e in situations where vimL is forbidden) and must use vim.schedule(callback) for anything that touches vimL or mutates editor state. I'm not 100% we want this, but if we do, we should introduce this early on.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Jun 4, 2017

Lua is no more thread-safe then VimL is, except that you can asynchronously use different lua_States and VimL has a bunch of globals scattered throught the code instead.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 4, 2017

I'm not thinking of threads, just event handling in the main thread. There are many circumstances where the uv event loop is being run, but most events (that can execute arbitrary vimscript, most API functions) are blocked. We could be more liberal with executing lua callbacks if this behavior is introduced from start. Alternatively they can by default behave like vimL callbacks, and this behavior behind a flag.

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 5, 2017

Now allows byte based sockets:

func! SockHandler(chan_id, data, event)
   let g:event = [a:chan_id, a:data, a:event]
endfunc

let x = sockconnect('pipe', '/some/nvim/socket`, {'on_output': 'SockHandler'})
call jobsend(x, "blargh")
echo g:event 

should see something like
[4, ['~@l^A~@1~@^A~@^A~@^A~@^A~@^A~@^A~@^A~@^A~@n^A~@IMessage is not an array~@@'], 'socket']

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 5, 2017

Also implement stdioopen({'rpc':..., 'on_stdin':...}). Only works in --headless mode, obviously. nvim --embed is then essentially a short-hand for nvim --headless --cmd "call stdioopen({'rpc': v:true})". Also stdio channel is guaranteed to be 1, so tests can just use 1 instead of needing to query nvim_get_api_info

uint64_t channel_from_stdio(bool rpc, CallbackReader on_output, const char **error)
{
if (!headless_mode) {
error = _("Stdio channel can only be opened in headless mode");

This comment has been minimized.

@oni-link

oni-link Jun 5, 2017

Contributor

*error instead of error

}
if (did_stdio) {
error = _("Stdio channel was already open");

This comment has been minimized.

@oni-link

oni-link Jun 5, 2017

Contributor

*error instead of error

@@ -23,13 +23,15 @@ struct process {
uint64_t stopped_time;
const char *cwd;
char **argv;
Stream *in, *out, *err;
Stream in, out, err;

This comment has been minimized.

@oni-link

oni-link Jun 5, 2017

Contributor

process_init() still initializes these members with NULL.

This comment has been minimized.

@bfredl

bfredl Jun 5, 2017

Member

That explains gcc:s strange warning message (it complains of missing braces for all of Process, not the Stream members).

@bfredl bfredl force-pushed the bfredl:channel branch 2 times, most recently from 4d60817 to e272633 Jun 5, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 6, 2017

Rather than allowing a separate close callback on_stdout: {"data": "Callback1", "close": "Callback2"}maybe be should just indicate this with special value, like data set to [] in the same callback. But it might be a clumsy with all-data-on-close behavior. The best It would also be good to distinguish signalling EOF in PTY input, maybe this can be data=[''].

Another solution is to rename the on_stdout/on_stderr argument, so if the new name is used, another flags argument will be received, but the old name gives compatible behavior.

@bfredl bfredl force-pushed the bfredl:channel branch from 6893a3e to 3b39fbd Jun 7, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 7, 2017

Implemented option stdout_buffered to get all output data at once (at EOF).

@bfredl bfredl force-pushed the bfredl:channel branch from 0b5978f to a15d50c Jun 8, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 8, 2017

Now jobclose(chan, part) works for all channels. Obviously it should be renamed, just like jobsend(). Currently we have

Functions that return channels

jobstart()
termopen()
sockconnect()
stdioopen()

Functions that take jobs

jobpid()
jobresize()
jobstop()
jobwait()

Functions that take any channel

rpcrequest()
rpcnotify()
jobsend()
jobclose()

(ignoring deprecated functions that can be done using any above)

TerminalJobData *data;
map_foreach_value(jobs, data, {
Channel *data;
map_foreach_value(channels, data, {
set_ref_in_callback(&data->on_stdout, copyID, NULL, NULL);

This comment has been minimized.

@oni-link

oni-link Jun 8, 2017

Contributor

Missing cast (Callback *) to hide -Wincompatible_pointer_types warning for first argument.

Perhaps also add somewhere a static_assert to make sure that the Callback object is the first member in CallbackReader:

#include "nvim/assert.h"
#include <stddef.h>
STATIC_ASSERT(offsetof(CallbackReader,cb)== 0,"...");

This comment has been minimized.

@bfredl

bfredl Jun 8, 2017

Member

I do get a warning already, I have just not fixed it as it is still WIP

TerminalJobData *data;
map_foreach_value(jobs, data, {
Channel *data;
map_foreach_value(channels, data, {
set_ref_in_callback(&data->on_stdout, copyID, NULL, NULL);
set_ref_in_callback(&data->on_stderr, copyID, NULL, NULL);

This comment has been minimized.

@oni-link

oni-link Jun 8, 2017

Contributor

Missing cast

.in = NULL,
.out = NULL,
.err = NULL,
.in = {0},

This comment has been minimized.

@oni-link

oni-link Jun 8, 2017

Contributor

Clang complains here about a missing initializer (field uv). If we remove .in = ..., .out =... and .err = ... the compiler should set the fields to zero.

This comment has been minimized.

@ZyX-I

ZyX-I Jun 8, 2017

Contributor

I prefer being explicit in such cases rather then rely on compiler zeroing things out.

This comment has been minimized.

@bfredl

bfredl Jun 8, 2017

Member

Strange that clang complains, {0} is a standard pattern and it's used elsewhere without complaints.

@bfredl bfredl force-pushed the bfredl:channel branch from a15d50c to f3c1630 Jun 8, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Jun 9, 2017

What is also missing is able to write to nvim's own stderr. It should probably not be mixed into the stdio channel as chansend(1, datas, "stderr") would be quite ugly, and also stderr is always possible even if stdin/out is not used as a channel. Rather, either a dedicated channel chansend(2, data) or a specific function write_stderr(data).

}
/// @param data will be consumed
size_t channel_send(uint64_t id, char* data, size_t len, const char **error)

This comment has been minimized.

@oni-link

oni-link Jun 9, 2017

Contributor

Why use size_t as return type? It looks like this function only returns 0 or 1.

This comment has been minimized.

@bfredl

bfredl Jun 9, 2017

Member

it should return the number of bytes written, as is standard for write function. But it looks like wstream_write doesn't do this.

This comment has been minimized.

@oni-link

oni-link Jun 9, 2017

Contributor

fwrite() only returns the number of items written (here 1). That number is also not equal the number of bytes written if len != 1.

Channel *channel = channel_alloc(kChannelTypeSocket);
if (!socket_connect(&main_loop, &channel->stream.socket,
tcp, address, timeout, error)) {
// TODO: early detroy that doesn't consume a channel id?

This comment has been minimized.

@oni-link

oni-link Jun 9, 2017

Contributor

typo: detroy

This comment has been minimized.

@bfredl

bfredl Jun 9, 2017

Member

A TODO without (name) is something I will fix before merging the PR.

// TODO: test mixed mode (server RPC, client bytes)
if (!tcp && rpc) {
char *path = fix_fname(address);
if (rpc && server_owns_pipe_address(path)) {

This comment has been minimized.

@oni-link

oni-link Jun 9, 2017

Contributor

rpc is here always true.

@bfredl bfredl force-pushed the bfredl:channel branch from 0e9d9a6 to b217974 Jun 10, 2017

@@ -117,8 +117,8 @@ variables:
- *b:term_title* The settable title of the terminal, typically displayed in
the window title or tab title of a graphical terminal emulator. Programs
running in the terminal can set this title via an escape sequence.
- *b:terminal_job_id* The nvim job ID of the job running in the terminal. See
|job-control| for more information.
- *b:terminal_job_id* The nvim channel ID or the PTY of the terminal emulateor. See

This comment has been minimized.

@brcolow

brcolow Jun 11, 2017

Contributor

typo: emulateor -> emulator

This comment has been minimized.

@oni-link

oni-link Jul 2, 2017

Contributor

Second half of the sentence not clear to me. What is the PTY of the terminal emulator?

This comment has been minimized.

@bfredl

bfredl Jul 2, 2017

Member

should be channel ID of the PTY of the emulator, but I guess it could be clearer; the channelrepresents specifically the master end of the PTY, not the PTY itself.

There are several ways to open a channel:
1. Through stdin/stdout when `nvim` is started with `--headless`, and a startup
script or --cmd commad opens the stdio channel using |stdioopen()|.

This comment has been minimized.

@brcolow

brcolow Jun 11, 2017

Contributor

typo: commad -> command

4. By connecting to a TCP/IP socket or named pipe with |sockconnect()|.
5. By another process connecting to a socket listened by nvim. This only

This comment has been minimized.

@brcolow

brcolow Jun 11, 2017

Contributor

socket listened by => socket listened to by

5. By another process connecting to a socket listened by nvim. This only
supports RPC channels, see |rpc-connecting|.
Channels can operate in two different modes, a mode using the |rpc| protocol,

This comment has been minimized.

@brcolow

brcolow Jun 11, 2017

Contributor

I would re-organize to:

Channels can operate in two modes: one via the |rpc| protocol (msgpack-rpc
based) and the other in "bytes" mode where scripts read and write raw bytes
over the channel. As a caveat even if a job's channel is in |rpc| mode, bytes can still
be read over its' stderr. Additionally only bytes can be written to nvim's stderr.

(Honestly it doesn't make a big difference either way, feel completely free to reject
this suggestion).

Edit: Is it possible to link to the next section after mentioning "bytes" mode?
Or is that too much?

This comment has been minimized.

@bfredl

bfredl Jun 11, 2017

Member

almost, but I prefer to think of stderr as an asset, not as a "caveat"

@bfredl bfredl force-pushed the bfredl:channel branch from d339088 to 0de019b Nov 26, 2017

@bfredl bfredl merged commit 207b7ca into neovim:master Nov 26, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
QuickBuild Build pr-6844 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bfredl

This comment has been minimized.

Member

bfredl commented Nov 26, 2017

merged. The "channels can connect to socket" (bytes mode) test was a bit flaky the other day, but has been fine the latest retries. If it causes trouble again I will mark it as pending (and eventually fix it by adding a proper bytes server shim, instead of abusing msgpackdump for this purpose).

@lambdalisue

This comment has been minimized.

lambdalisue commented Nov 30, 2017

This seems breaking old behavior... An extra newline seems to be added. I'll investigate deeply later.

https://travis-ci.org/lambdalisue/gina.vim/jobs/308500736#L2077
https://ci.appveyor.com/project/lambdalisue/gina-vim/build/655/job/brq82xko9xudoi8p#L115

@bfredl

This comment has been minimized.

Member

bfredl commented Nov 30, 2017

Hmm, I first though it was issue of the extra EOF event that is added. But gina#util#extend_content() looks like it should handle it perfectly fine. ((unrelated note: you can avoid the extra branch by initializing job.stdout/stderr with [''] instead of []) Can you confirm it is what's been used in this test and not some other stuff, in vital for instance?

@lambdalisue

This comment has been minimized.

lambdalisue commented Dec 1, 2017

@lambdalisue

This comment has been minimized.

lambdalisue commented Dec 1, 2017

I first though it was issue of the extra EOF event that is added.

Do you have detail? I would like to investigate that.

Can you confirm it is what's been used in this test and not some other stuff, in vital for instance?

Possible. I'll check thanks 👍

@lambdalisue

This comment has been minimized.

lambdalisue commented Dec 1, 2017

https://github.com/lambdalisue/gina.vim/blob/master/test/gina/process.vimspec#L31

You were correct. The test has its own inplementation which just add the data to a list.

So is this behavior change intentional? And is there any documentation about behavior change?

@justinmk

This comment has been minimized.

Member

justinmk commented Dec 1, 2017

@lambdalisue See "Note 2" in :help job_control. The behavior didn't change, it may have worked by chance before.

@bfredl

This comment has been minimized.

Member

bfredl commented Dec 1, 2017

@justinmk The behavior did change so that EOF is identifiable, but it should be purely an information gain change. 90 % of plugins that uses the recommended pattern should notice no breaking change, 9% was broken already but extremely unlikely seen because flushing behavior is kind of deterministic even if no guarantees actually was made.1% is the "spacebar heating" cases we need to give recommendations individually.

@wsdjeg

This comment has been minimized.

Contributor

wsdjeg commented Dec 1, 2017

90 % of plugins that uses the recommended pattern should notice no breaking change

so what is the recommanded pattern ?

if a shell command output in terminal is:

foo

foo
foo

then the a:data of stdout func is ['foo', '', 'foo', 'foo']?

and any document show the differences, or any job example show the differences?

@lambdalisue

This comment has been minimized.

lambdalisue commented Dec 1, 2017

90 % of plugins that uses the recommended pattern should notice no breaking change

In my case, I was using a low-level implementation (only in one test, other parts I was using a recommended pattern) so the test had failed.
Once I changed the implementation as an example in a help, the test pass 🎉

lambdalisue/gina.vim@0bdb844

@wsdjeg

Prob "Note 2" in :help job_control is the recommended pattern.

@wsdjeg

This comment has been minimized.

Contributor

wsdjeg commented Dec 1, 2017

@lambdalisue Thanks, I got it.

@wsdjeg

This comment has been minimized.

Contributor

wsdjeg commented Dec 1, 2017

@bfredl is there a check flag for this change? just to make sure my config is compatibility to old version of neovim, for example

   " some new func added in this pr
   if exist('*new_func_name')
       " old stdout handle func
   else
      " new fixed stdout handle func
   endif
@bfredl

This comment has been minimized.

Member

bfredl commented Dec 1, 2017

The point is that a function that parses lines correctly, shouldn't need to change behavior depending on version. But you can of course check for chansend() existing if really want.

lambdalisue added a commit to lambdalisue/vital-System-Job that referenced this pull request Jan 15, 2018

Prefer new names for jobsend/jobclose
jobsend/jobclose has renamed to chansend/chanclose in Neovim 0.2.3 and the
old names became obsolute.

PR: neovim/neovim#6844
// We want to deal with stream events as fast a possible while queueing
// process events, so reset everything to NULL. It prevents closing the
// Note: unlike process events, stream events are not queued, as we want to
// deal with stream events as fast a possible. It prevents closing the
// streams while there's still data in the OS buffer (due to the process

This comment has been minimized.

@justinmk

justinmk Feb 1, 2018

Member

Does "It prevents" still make sense here? What prevents the streams from closing?

@boxofrox

This comment has been minimized.

boxofrox commented Feb 10, 2018

Are there any plans to extend sockconnect to work without the RPC flag? I found a use-case for such a feature while working with vim-picker.

@bfredl

This comment has been minimized.

Member

bfredl commented Feb 10, 2018

@boxofrox This is already implemented, but only on master.

@bfredl bfredl referenced this pull request Mar 19, 2018

Merged

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

5 of 6 tasks complete

justinmk added a commit that referenced this pull request Jun 11, 2018

NVIM v0.3.0
FEATURES:
3cc7ebf #7234 built-in VimL expression parser
6a7c904 #4419 implement <Cmd> key to invoke command in any mode
b836328 #7679 'startup: treat stdin as text instead of commands'
58b210e :digraphs : highlight with hl-SpecialKey #2690
7a13611 #8276 'startup: Let `-s -` read from stdin'
1e71978 events: VimSuspend, VimResume #8280
1e7d5e8 #6272 'stdpath()'
f96d99a #8247 server: introduce --listen
e8c39f7 #8226 insert-mode: interpret unmapped META as ESC
98e7112 msg: do not scroll entire screen (#8088)
f72630b #8055 let negative 'writedelay' show all redraws
5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330
a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422)
207b7ca #6844 channels: support buffered output and bytes sockets/stdio

API:
f85cbea #7917 API: buffer updates
418abfc #6743 API: list information about all channels/jobs.
36b2e3f #8375 API: nvim_get_commands
273d2cd #8329 API: Make nvim_set_option() update `:verbose set …`
8d40b36 #8371 API: more reliable/descriptive VimL errors
ebb1acb #8353 API: nvim_call_dict_function
9f994bb #8004 API: nvim_list_uis
3405704 #7520 API/UI: forward option updates to UIs
911b1e4 #7821 API: improve nvim_command_output

WINDOWS OS:
9cefd83 #8084, #8516 build/win: support MSVC
ee4e1fd win: Fix reading content from stdin (#8267)

TUI:
ffb8904 #8309 TUI: add support for mouse release events in urxvt
8d5a46e #8081 TUI: implement "standout" attribute
6071637 TUI: support TERM=konsole-256color
67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3)
3d0ee17 TUI/rxvt: enable focus-reporting
d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior

FIXES:
ed6a113 #8273 'job-control: avoid kill-timer race'
4e02f1a #8107 'jobs: separate process-group'
451c48a terminal: flush vterm output buffer on pty output #8486
5d6732f :checkhealth fixes #8335
53f11dc #8218 'Fix errors reported by PVS'
d05712f inccommand: pause :terminal redraws (#8307)
51af911 inccommand: do not execute trailing commands #8256
84359a4 terminal: resize to the max dimensions (#8249)
d49c1dd #8228 Make vim_fgets() return the same values as in Vim
60e96a4 screen: winhl=Normal:Background should not override syntax (#8093)
0c59ac1 #5908 'shada: Also save numbered marks'
ba87a2c cscope: ignore EINTR while reading the prompt (#8079)
b1412dc #7971 ':terminal Enter/Leave should not increment jumplist'
3a5721e TUI: libtermkey: force CSI driver for mouse input #7948
6ff13d7 #7720 TUI: faster startup
1c6e956 #7862 TUI: fix resize-related segfaults
a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output
303e1df #7624 TUI: disable BCE almost always
249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum
6f41ce0 #7704 macOS: Set $LANG based on the system locale
a043899 #7633 'Retry fgets on EINTR'

CHANGES:
ad60927 #8304 default to 'nofsync'
f3f1970 #8035 defaults: 'fillchars'
a6052c7 #7984 defaults: sidescroll=1
b69fa86 #7888 defaults: enable cscopeverbose
7c4bb23 defaults: do :filetype stuff unless explicitly "off"
2aa308c #5658 'Apply :lmap in macros'
8ce6393 terminal: Leave 'relativenumber' alone (#8360)
e46534b #4486 refactor: Remove maxmem, maxmemtot options
131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343
c57d315 #8031 jobwait(): return -2 on interrupt also with timeout
6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940
300d365 #7919 Make 'langnoremap' apply directly after a map
ada1956 #7880 'lua/executor: Remove lightuserdata'

INTERNAL:
de0a954 #7806 internal statistics for list impl
dee78a4 #7708 rewrite internal list impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment