From 8a782f1699e2a59a3f3e91f6d7c35a3312b82b41 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Thu, 21 May 2015 09:45:46 -0400 Subject: [PATCH 1/3] input: set input stream to blocking on exit If stdin is non-blocking, many tools (e.g. cat(1), read(1)) which assume that stdin is blocking, will break in odd ways: read: read error: 0: Resource temporarily unavailable cat: -: Resource temporarily unavailable rm: error closing file libuv puts stdin in nonblocking mode, and leaves it that way at exit (this is apparently by design). So, before this commit, this always works (because the shell clobbers O_NONBLOCK): $ nvim --cmd q $ read ...but these forms do _not_ work: $ nvim --cmd q && read $ echo foo | nvim --cmd q && read $ nvim && read After this commit, all of the above forms work. Background: https://github.com/fish-shell/fish-shell/commit/437b4397b9cf273922ce7b414bf6626845f15ad0#diff-41f4d294430cd8c36538999d62681ae2 https://github.com/fish-shell/fish-shell/issues/176#issuecomment-15800155 - bash (and other shells: zsh, tcsh, fish), upon returning to the foreground, always sets fd 0 back to blocking mode. This practice only applies to stdin, _not_ stdout or stderr (in practice these fds may be affected anyways). - bash/zsh/tcsh/fish do _not_ restore the non-blocking status of stdin when _resuming a job_. - We do _not_ save/restore the original flags visible to fcntl(F_[SG]ETFL), because (counterintuitively) that isn't expected. Helped-by: oni-link Closes #2086 Closes #2377 --- Note: The following implementation of stream_set_blocking() was discarded, because it resulted in a failed libuv assertion[1]: int stream_set_blocking(int fd, bool blocking) { uv_pipe_t stream; uv_pipe_init(uv_default_loop(), &stream, 0); uv_pipe_open(&stream, fd); int retval = uv_stream_set_blocking((uv_stream_t *)&stream, blocking); uv_close((uv_handle_t *)&stream, NULL); return retval; } [1] .deps/build/src/libuv/src/unix/core.c:833: uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed. --- src/nvim/globals.h | 3 +++ src/nvim/if_cscope.c | 1 + src/nvim/main.c | 16 +++++++--------- src/nvim/misc1.c | 1 + src/nvim/os/input.c | 6 ++++++ src/nvim/os/os.h | 1 + src/nvim/os/rstream.c | 5 +++++ src/nvim/os/stream.c | 26 ++++++++++++++++++++++++++ 8 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 src/nvim/os/stream.c diff --git a/src/nvim/globals.h b/src/nvim/globals.h index 1d93900a94..f012358404 100644 --- a/src/nvim/globals.h +++ b/src/nvim/globals.h @@ -1217,6 +1217,9 @@ EXTERN char *ignoredp; // If a msgpack-rpc channel should be started over stdin/stdout EXTERN bool embedded_mode INIT(= false); +/// TTY from which input was gathered. +EXTERN int global_input_fd INIT(= 0); + /// Used to track the status of external functions. /// Currently only used for iconv(). typedef enum { diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index 407709866c..b3d61038c3 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -820,6 +820,7 @@ err_closing: if (execl("/bin/sh", "sh", "-c", cmd, (char *)NULL) == -1) PERROR(_("cs_create_connection exec failed")); + stream_set_blocking(global_input_fd, true); //normalize stream (#2598) exit(127); /* NOTREACHED */ default: /* parent. */ diff --git a/src/nvim/main.c b/src/nvim/main.c index 5f0372f30e..6ae96a7c69 100644 --- a/src/nvim/main.c +++ b/src/nvim/main.c @@ -271,17 +271,15 @@ int main(int argc, char **argv) || params.output_isatty || params.err_isatty); if (reading_input) { - // Its possible that one of the startup commands(arguments, sourced scripts - // or plugins) will prompt the user, so start reading from a tty stream - // now. - int fd = fileno(stdin); + // One of the startup commands (arguments, sourced scripts or plugins) may + // prompt the user, so start reading from a tty now. + global_input_fd = fileno(stdin); if (!params.input_isatty || params.edit_type == EDIT_STDIN) { - // use stderr or stdout since stdin is not a tty and/or could be used to - // read the file we'll edit when the "-" argument is given(eg: cat file | - // nvim -) - fd = params.err_isatty ? fileno(stderr) : fileno(stdout); + // Use stderr or stdout since stdin is not a tty and/or could be used to + // read the "-" file (eg: cat file | nvim -) + global_input_fd = params.err_isatty ? fileno(stderr) : fileno(stdout); } - input_start_stdin(fd); + input_start_stdin(global_input_fd); } // open terminals when opening files that start with term:// diff --git a/src/nvim/misc1.c b/src/nvim/misc1.c index 3ae3d6e30e..13e5f273e0 100644 --- a/src/nvim/misc1.c +++ b/src/nvim/misc1.c @@ -2699,6 +2699,7 @@ void preserve_exit(void) } ml_close_all(false); // close all memfiles, without deleting + stream_set_blocking(global_input_fd, true); //normalize stream (#2598) mch_errmsg("Vim: Finished.\n"); diff --git a/src/nvim/os/input.c b/src/nvim/os/input.c index 486171b48a..5dcaee8304 100644 --- a/src/nvim/os/input.c +++ b/src/nvim/os/input.c @@ -8,6 +8,7 @@ #include "nvim/api/private/defs.h" #include "nvim/os/input.h" #include "nvim/os/event.h" +#include "nvim/os/os.h" #include "nvim/os/rstream_defs.h" #include "nvim/os/rstream.h" #include "nvim/ascii.h" @@ -61,9 +62,14 @@ void input_start_stdin(int fd) void input_stop_stdin(void) { if (!read_stream) { + // In some cases (i.e. "nvim && read") where we do not explicitly play with + // std{in,out,err}, some other module/library nevertheless sets the stream + // to non-blocking; we still must "fix" the stream (#2598) in those cases. + stream_set_blocking(global_input_fd, true); // normalize stream (#2598) return; } + rstream_set_blocking(read_stream, true); // normalize stream (#2598) rstream_stop(read_stream); rstream_free(read_stream); read_stream = NULL; diff --git a/src/nvim/os/os.h b/src/nvim/os/os.h index 69bd1ff4fd..3dd099890c 100644 --- a/src/nvim/os/os.h +++ b/src/nvim/os/os.h @@ -12,6 +12,7 @@ # include "os/mem.h.generated.h" # include "os/env.h.generated.h" # include "os/users.h.generated.h" +# include "os/stream.h.generated.h" #endif #endif // NVIM_OS_OS_H diff --git a/src/nvim/os/rstream.c b/src/nvim/os/rstream.c index 702f282d53..55bf89d768 100644 --- a/src/nvim/os/rstream.c +++ b/src/nvim/os/rstream.c @@ -297,6 +297,11 @@ void rstream_stop(RStream *rstream) } } +int rstream_set_blocking(RStream *rstream, bool blocking) +{ + return uv_stream_set_blocking(rstream->stream, blocking); +} + /// Returns the number of bytes ready for consumption in `rstream` size_t rstream_pending(RStream *rstream) { diff --git a/src/nvim/os/stream.c b/src/nvim/os/stream.c new file mode 100644 index 0000000000..35cb41081d --- /dev/null +++ b/src/nvim/os/stream.c @@ -0,0 +1,26 @@ +// Functions for working with stdio streams (as opposed to RStream/WStream). + +#include +#include + +#include + +#ifdef INCLUDE_GENERATED_DECLARATIONS +# include "os/stream.c.generated.h" +#endif + +/// Sets the stream associated with `fd` to "blocking" mode. +/// +/// @return `0` on success, or `-errno` on failure. +int stream_set_blocking(int fd, bool blocking) +{ + int flags = fcntl(fd, F_GETFL, 0); + int err = 0; + if (!blocking && !(flags & O_NONBLOCK)) { + err = fcntl(fd, F_SETFL, flags | O_NONBLOCK); + } else if (blocking && (flags & O_NONBLOCK)) { + err = fcntl(fd, F_SETFL, flags & ~O_NONBLOCK); + } + return err == -1 ? -errno : 0; +} + From 4219b69145f13e95eadb5a666cb02b482bdd395b Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 25 May 2015 14:13:18 -0400 Subject: [PATCH 2/3] input: stream_set_blocking(): libuv impl - Create a private libuv loop instead of re-using uv_default_loop(), to avoid conflict[1] with existing watcher(s) on the fd. - Expose the global "input" fd as a getter instead of a mutable global. [1] .deps/build/src/libuv/src/unix/core.c:833: uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed. --- src/nvim/globals.h | 3 --- src/nvim/if_cscope.c | 2 +- src/nvim/main.c | 6 +++--- src/nvim/misc1.c | 2 +- src/nvim/os/input.c | 13 ++++++++----- src/nvim/os/rstream.c | 5 ----- src/nvim/os/stream.c | 20 ++++++++++++-------- src/nvim/os_unix.c | 1 + 8 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/nvim/globals.h b/src/nvim/globals.h index f012358404..1d93900a94 100644 --- a/src/nvim/globals.h +++ b/src/nvim/globals.h @@ -1217,9 +1217,6 @@ EXTERN char *ignoredp; // If a msgpack-rpc channel should be started over stdin/stdout EXTERN bool embedded_mode INIT(= false); -/// TTY from which input was gathered. -EXTERN int global_input_fd INIT(= 0); - /// Used to track the status of external functions. /// Currently only used for iconv(). typedef enum { diff --git a/src/nvim/if_cscope.c b/src/nvim/if_cscope.c index b3d61038c3..99926ecf16 100644 --- a/src/nvim/if_cscope.c +++ b/src/nvim/if_cscope.c @@ -820,7 +820,7 @@ err_closing: if (execl("/bin/sh", "sh", "-c", cmd, (char *)NULL) == -1) PERROR(_("cs_create_connection exec failed")); - stream_set_blocking(global_input_fd, true); //normalize stream (#2598) + stream_set_blocking(input_global_fd(), true); // normalize stream (#2598) exit(127); /* NOTREACHED */ default: /* parent. */ diff --git a/src/nvim/main.c b/src/nvim/main.c index 6ae96a7c69..4714de7c59 100644 --- a/src/nvim/main.c +++ b/src/nvim/main.c @@ -273,13 +273,13 @@ int main(int argc, char **argv) if (reading_input) { // One of the startup commands (arguments, sourced scripts or plugins) may // prompt the user, so start reading from a tty now. - global_input_fd = fileno(stdin); + int fd = fileno(stdin); if (!params.input_isatty || params.edit_type == EDIT_STDIN) { // Use stderr or stdout since stdin is not a tty and/or could be used to // read the "-" file (eg: cat file | nvim -) - global_input_fd = params.err_isatty ? fileno(stderr) : fileno(stdout); + fd = params.err_isatty ? fileno(stderr) : fileno(stdout); } - input_start_stdin(global_input_fd); + input_start_stdin(fd); } // open terminals when opening files that start with term:// diff --git a/src/nvim/misc1.c b/src/nvim/misc1.c index 13e5f273e0..0737caec5d 100644 --- a/src/nvim/misc1.c +++ b/src/nvim/misc1.c @@ -2679,6 +2679,7 @@ void preserve_exit(void) // Prevent repeated calls into this method. if (really_exiting) { + stream_set_blocking(input_global_fd(), true); //normalize stream (#2598) exit(2); } @@ -2699,7 +2700,6 @@ void preserve_exit(void) } ml_close_all(false); // close all memfiles, without deleting - stream_set_blocking(global_input_fd, true); //normalize stream (#2598) mch_errmsg("Vim: Finished.\n"); diff --git a/src/nvim/os/input.c b/src/nvim/os/input.c index 5dcaee8304..f89ca4af95 100644 --- a/src/nvim/os/input.c +++ b/src/nvim/os/input.c @@ -35,6 +35,7 @@ typedef enum { static RStream *read_stream = NULL; static RBuffer *read_buffer = NULL, *input_buffer = NULL; static bool input_eof = false; +static int global_fd = 0; #ifdef INCLUDE_GENERATED_DECLARATIONS # include "os/input.c.generated.h" @@ -47,12 +48,19 @@ void input_init(void) input_buffer = rbuffer_new(INPUT_BUFFER_SIZE + MAX_KEY_CODE_LEN); } +/// Gets the file from which input was gathered at startup. +int input_global_fd(void) +{ + return global_fd; +} + void input_start_stdin(int fd) { if (read_stream) { return; } + global_fd = fd; read_buffer = rbuffer_new(READ_BUFFER_SIZE); read_stream = rstream_new(read_cb, read_buffer, NULL); rstream_set_file(read_stream, fd); @@ -62,14 +70,9 @@ void input_start_stdin(int fd) void input_stop_stdin(void) { if (!read_stream) { - // In some cases (i.e. "nvim && read") where we do not explicitly play with - // std{in,out,err}, some other module/library nevertheless sets the stream - // to non-blocking; we still must "fix" the stream (#2598) in those cases. - stream_set_blocking(global_input_fd, true); // normalize stream (#2598) return; } - rstream_set_blocking(read_stream, true); // normalize stream (#2598) rstream_stop(read_stream); rstream_free(read_stream); read_stream = NULL; diff --git a/src/nvim/os/rstream.c b/src/nvim/os/rstream.c index 55bf89d768..702f282d53 100644 --- a/src/nvim/os/rstream.c +++ b/src/nvim/os/rstream.c @@ -297,11 +297,6 @@ void rstream_stop(RStream *rstream) } } -int rstream_set_blocking(RStream *rstream, bool blocking) -{ - return uv_stream_set_blocking(rstream->stream, blocking); -} - /// Returns the number of bytes ready for consumption in `rstream` size_t rstream_pending(RStream *rstream) { diff --git a/src/nvim/os/stream.c b/src/nvim/os/stream.c index 35cb41081d..0c448872c3 100644 --- a/src/nvim/os/stream.c +++ b/src/nvim/os/stream.c @@ -14,13 +14,17 @@ /// @return `0` on success, or `-errno` on failure. int stream_set_blocking(int fd, bool blocking) { - int flags = fcntl(fd, F_GETFL, 0); - int err = 0; - if (!blocking && !(flags & O_NONBLOCK)) { - err = fcntl(fd, F_SETFL, flags | O_NONBLOCK); - } else if (blocking && (flags & O_NONBLOCK)) { - err = fcntl(fd, F_SETFL, flags & ~O_NONBLOCK); - } - return err == -1 ? -errno : 0; + // Private loop to avoid conflict with existing watcher(s): + // uv__io_stop: Assertion `loop->watchers[w->fd] == w' failed. + uv_loop_t loop; + uv_pipe_t stream; + uv_loop_init(&loop); + uv_pipe_init(&loop, &stream, 0); + uv_pipe_open(&stream, fd); + int retval = uv_stream_set_blocking((uv_stream_t *)&stream, blocking); + uv_close((uv_handle_t *)&stream, NULL); + uv_run(&loop, UV_RUN_NOWAIT); // not necessary, but couldn't hurt. + uv_loop_close(&loop); + return retval; } diff --git a/src/nvim/os_unix.c b/src/nvim/os_unix.c index b2d5bae477..ccd0073db1 100644 --- a/src/nvim/os_unix.c +++ b/src/nvim/os_unix.c @@ -181,6 +181,7 @@ void mch_exit(int r) ml_close_all(TRUE); /* remove all memfiles */ event_teardown(); + stream_set_blocking(input_global_fd(), true); // normalize stream (#2598) #ifdef EXITFREE free_all_mem(); From b2c400b3f2354b2765e1d6f3b5ce4b11f2ab75a3 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 25 May 2015 14:31:12 -0400 Subject: [PATCH 3/3] input: rename input_{start,stop}_stdin() - "stdin" is misleading because it may read from stdout or stderr - also remove some unused includes --- src/nvim/main.c | 6 +++--- src/nvim/os/event.c | 2 +- src/nvim/os/input.c | 6 ++---- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/nvim/main.c b/src/nvim/main.c index 4714de7c59..fc72039b5f 100644 --- a/src/nvim/main.c +++ b/src/nvim/main.c @@ -279,7 +279,7 @@ int main(int argc, char **argv) // read the "-" file (eg: cat file | nvim -) fd = params.err_isatty ? fileno(stderr) : fileno(stdout); } - input_start_stdin(fd); + input_start(fd); } // open terminals when opening files that start with term:// @@ -386,8 +386,8 @@ int main(int argc, char **argv) } if (!params.headless) { - // Stop reading from stdin, the UI layer will take over now - input_stop_stdin(); + // Stop reading from input stream, the UI layer will take over now. + input_stop(); ui_builtin_start(); } diff --git a/src/nvim/os/event.c b/src/nvim/os/event.c index 0560da1e2e..b0bd7ca55a 100644 --- a/src/nvim/os/event.c +++ b/src/nvim/os/event.c @@ -73,7 +73,7 @@ void event_teardown(void) // is required because the `process_events_from` above may call `event_push` // which will set the stop_flag to 1(uv_stop) uv_default_loop()->stop_flag = 0; - input_stop_stdin(); + input_stop(); channel_teardown(); job_teardown(); server_teardown(); diff --git a/src/nvim/os/input.c b/src/nvim/os/input.c index f89ca4af95..74a5d3bc2e 100644 --- a/src/nvim/os/input.c +++ b/src/nvim/os/input.c @@ -1,6 +1,5 @@ #include #include -#include #include #include @@ -8,7 +7,6 @@ #include "nvim/api/private/defs.h" #include "nvim/os/input.h" #include "nvim/os/event.h" -#include "nvim/os/os.h" #include "nvim/os/rstream_defs.h" #include "nvim/os/rstream.h" #include "nvim/ascii.h" @@ -54,7 +52,7 @@ int input_global_fd(void) return global_fd; } -void input_start_stdin(int fd) +void input_start(int fd) { if (read_stream) { return; @@ -67,7 +65,7 @@ void input_start_stdin(int fd) rstream_start(read_stream); } -void input_stop_stdin(void) +void input_stop(void) { if (!read_stream) { return;