feat(job): add parameter to close stdin

[gpanders]

Some programs behave differently when they detect that stdin is being
piped. This can be problematic when these programs are used with the job
control API where stdin is attached, but not typically used. It is
possible to run the job using a PTY which circumvents this problem, but
that includes a lot of overhead when simply closing the stdin pipe would
suffice.

To enable this behavior, add a new parameter to the jobstart options
dict called "stdin" with two valid values: "pipe" (the default)
implements the existing behavior of opening a channel for stdin and
"null" which disconnects stdin (or, if you prefer, connects it to
/dev/null). This is extensible so that other modes can be added in the
future.

Context

This was inspired by a recent change in ripgrep where the behavior of ripgrep changes if it detects that stdin is open. In the linked issue, the ripgrep maintainer rightfully asks the question, "Why is it attaching a socket to stdin in the first place?" (see also vim/vim#4529).

By default Neovim always attachesstdin and only attaches stdout and stderr if the appropriate callbacks are specified. Presently, the only way to disable automatically opening a stdin channel is to run the command in a pty using the 'pty' option; however, this adds a lot of overhead to simply avoid opening a stdin channel. This PR modifies that functionality by allowing the stdin channel to be closed (or rather, never be opened) if the "stdin" option is set to "null" in the options dict passed to jobstart.

[BurntSushi]

@gpanders Out of curiosity, have you confirmed that closing stdin here would cause ripgrep to do the right thing?

[gpanders]

@gpanders Out of curiosity, have you confirmed that closing stdin here would cause ripgrep to do the right thing?

I have and it does.

[bfredl]

should we call it stdin="closed" in case we want to support more modes later (like direct read from file, like vim8)?

[gpanders]

should we call it stdin="closed" in case we want to support more modes later (like direct read from file, like vim8)?

Sure, I like that. What do you want to call the current/default behavior? I'm thinking either 'open', 'attached', or 'pipe' (to match Vim).

[gpanders]

@bfredl I changed stdin_closed to a string parameter stdin that takes one of two values "pipe" or "null". Using "null" disconnects stdin (the same as setting stdin_closed to true) and using "pipe" (the default) implements the existing behavior. "pipe" and "null" mirror the names used in Vim.

[bfredl]

sure, if the name choices picked by vim8 is reasonable, we could just as well reuse them :)

[gpanders]

The CI checks seem to be failing for something unrelated to this PR

[bfredl]

A simple test would be good. doesn't need to be complicated, could just be jobstart() and then chansend() failing because channel already closed.

[gpanders]

I modified the it("disallows jobsend on a job that closed stdin") spec to add a test case with stdin = 'null'.

[justinmk]

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.

[gpanders]

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"?

[justinmk]

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.

[gpanders]

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.

[justinmk]

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

[gpanders]

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.

[bfredl]

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).