Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(job): add parameter to close stdin #14812

Merged
merged 4 commits into from
Jul 13, 2021
Merged

feat(job): add parameter to close stdin #14812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb7f24b
feat(job): add parameter to close stdin
gpanders Jun 14, 2021
35e13df
Rename stdin to stdin_mode
gpanders Jul 12, 2021
d738247
Fix line length clint error
gpanders Jul 12, 2021
f83763b
Add test case for 'null' stdin mode
gpanders Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions runtime/doc/eval.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5637,6 +5637,9 @@ jobstart({cmd}[, {opts}]) *jobstart()*
before invoking `on_stderr`. |channel-buffered|
stdout_buffered: (boolean) Collect data until EOF (stream
closed) before invoking `on_stdout`. |channel-buffered|
stdin: (string) Either "pipe" (default) to connect the
job's stdin to a channel or "null" to disconnect
stdin.
width: (number) Width of the `pty` terminal.

{opts} is passed as |self| dictionary to the callback; the
Expand Down
23 changes: 19 additions & 4 deletions src/nvim/channel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,9 @@ static void close_cb(Stream *stream, void *data)
/// `on_stdout` is ignored
/// @param[in] detach True if the job should not be killed when nvim exits,
/// ignored if `pty` is true
/// @param[in] stdin_mode Stdin mode. Either kChannelStdinPipe to open a
/// channel for stdin or kChannelStdinNull to leave
/// stdin disconnected.
/// @param[in] cwd Initial working directory for the job. Nvim's working
/// directory if `cwd` is NULL
/// @param[in] pty_width Width of the pty, ignored if `pty` is false
Expand All @@ -302,7 +305,7 @@ static void close_cb(Stream *stream, void *data)
Channel *channel_job_start(char **argv, CallbackReader on_stdout,
CallbackReader on_stderr, Callback on_exit,
bool pty, bool rpc, bool overlapped, bool detach,
const char *cwd,
ChannelStdinMode stdin_mode, const char *cwd,
uint16_t pty_width, uint16_t pty_height,
dict_T *env, varnumber_T *status_out)
{
Expand Down Expand Up @@ -345,15 +348,25 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
proc->overlapped = overlapped;

char *cmd = xstrdup(proc->argv[0]);
bool has_out, has_err;
bool has_in, has_out, has_err;
if (proc->type == kProcessTypePty) {
has_out = true;
has_err = false;
} else {
has_out = rpc || callback_reader_set(chan->on_data);
has_err = callback_reader_set(chan->on_stderr);
}
int status = process_spawn(proc, true, has_out, has_err);

switch (stdin_mode) {
case kChannelStdinPipe:
has_in = true;
break;
case kChannelStdinNull:
has_in = false;
break;
}

int status = process_spawn(proc, has_in, has_out, has_err);
if (status) {
EMSG3(_(e_jobspawn), os_strerror(status), cmd);
xfree(cmd);
Expand All @@ -369,7 +382,9 @@ Channel *channel_job_start(char **argv, CallbackReader on_stdout,
tv_dict_free(proc->env);
}

wstream_init(&proc->in, 0);
if (has_in) {
wstream_init(&proc->in, 0);
}
if (has_out) {
rstream_init(&proc->out, 0);
}
Expand Down
4 changes: 4 additions & 0 deletions src/nvim/channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ typedef enum {
kChannelPartAll
} ChannelPart;

typedef enum {
kChannelStdinPipe,
kChannelStdinNull,
} ChannelStdinMode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to name this ChannelMode, and add kChannelBuffered, then use this internally to model the stderr_buffered/stdout_buffered flags? And potentially we would add stderr, stdout string options to jobstart(), and deprecate the stderr_buffered/stdout_buffered booleans.

Copy link
Member Author

@gpanders gpanders Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that would make sense, as that introduces more edge cases we'd need to check for.

For example, what happens when "stdout" is set to "null" but an "on_stdout" callback is provided? What does it mean when "stdin" is set to "buffered"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, what happens when "stdout" is set to "null" but an "on_stdout" callback is provided? What does it mean when "stdin" is set to "buffered"?

At the input layer we would of course validate valid values, just as "foo" is not a valid value for the stdin mode parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, but the point I was trying to make was that I don't know if the additional validation required results in a net reduction in complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only make sense if we actually add more flags to the jobstart() interface (e.g stdout="file").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could put most of the validation responsibility on the user. We just return an error if "stdout" is anything other than "null" and "on_stdout" is missing and vice versa.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this makes sense for the existing feature set, but perhaps it does in a PR adding "buffer" and "file" modes (so there actually are shared modes between instreams and outsteams).


typedef struct {
Stream in;
Expand Down
26 changes: 20 additions & 6 deletions src/nvim/eval/funcs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5182,6 +5182,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)
bool pty = false;
bool clear_env = false;
bool overlapped = false;
ChannelStdinMode stdin_mode = kChannelStdinPipe;
CallbackReader on_stdout = CALLBACK_READER_INIT,
on_stderr = CALLBACK_READER_INIT;
Callback on_exit = CALLBACK_NONE;
Expand All @@ -5196,6 +5197,17 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)
clear_env = tv_dict_get_number(job_opts, "clear_env") != 0;
overlapped = tv_dict_get_number(job_opts, "overlapped") != 0;

char *s = tv_dict_get_string(job_opts, "stdin", false);
if (s) {
if (!strncmp(s, "null", NUMBUFLEN)) {
stdin_mode = kChannelStdinNull;
} else if (!strncmp(s, "pipe", NUMBUFLEN)) {
// Nothing to do, default value
} else {
EMSG3(_(e_invargNval), "stdin", s);
}
}

if (pty && rpc) {
EMSG2(_(e_invarg2), "job cannot have both 'pty' and 'rpc' options set");
shell_free_argv(argv);
Expand Down Expand Up @@ -5252,8 +5264,8 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)
env = create_environment(job_env, clear_env, pty, term_name);

Channel *chan = channel_job_start(argv, on_stdout, on_stderr, on_exit, pty,
rpc, overlapped, detach, cwd, width, height,
env, &rettv->vval.v_number);
rpc, overlapped, detach, stdin_mode, cwd,
width, height, env, &rettv->vval.v_number);
if (chan) {
channel_create_event(chan, NULL);
}
Expand Down Expand Up @@ -7733,8 +7745,9 @@ static void f_rpcstart(typval_T *argvars, typval_T *rettv, FunPtr fptr)

Channel *chan = channel_job_start(argv, CALLBACK_READER_INIT,
CALLBACK_READER_INIT, CALLBACK_NONE,
false, true, false, false, NULL, 0, 0,
NULL, &rettv->vval.v_number);
false, true, false, false,
kChannelStdinPipe, NULL, 0, 0, NULL,
&rettv->vval.v_number);
if (chan) {
channel_create_event(chan, NULL);
}
Expand Down Expand Up @@ -10850,10 +10863,11 @@ static void f_termopen(typval_T *argvars, typval_T *rettv, FunPtr fptr)
const bool rpc = false;
const bool overlapped = false;
const bool detach = false;
ChannelStdinMode stdin_mode = kChannelStdinPipe;
uint16_t term_width = MAX(0, curwin->w_width_inner - win_col_off(curwin));
Channel *chan = channel_job_start(argv, on_stdout, on_stderr, on_exit,
pty, rpc, overlapped, detach, cwd,
term_width, curwin->w_height_inner,
pty, rpc, overlapped, detach, stdin_mode,
cwd, term_width, curwin->w_height_inner,
env, &rettv->vval.v_number);
if (rettv->vval.v_number <= 0) {
return;
Expand Down
6 changes: 6 additions & 0 deletions test/functional/core/job_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,12 @@ describe('jobs', function()
eq(false, pcall(function()
nvim('command', 'call jobsend(j, ["some data"])')
end))

command("let g:job_opts.stdin = 'null'")
nvim('command', "let j = jobstart(['cat', '-'], g:job_opts)")
eq(false, pcall(function()
nvim('command', 'call jobsend(j, ["some data"])')
end))
end)

it('disallows jobsend on a non-existent job', function()
Expand Down