From ef4676ed5bd18fc2d5fb52b52304d59d94e9e806 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 11 Oct 2016 23:14:18 +0200 Subject: test/terminal: Cover race when :term shell process exits. References #5445 See https://github.com/neovim/neovim/pull/5445#issuecomment-252529766 --- src/nvim/terminal.c | 2 +- test/functional/terminal/ex_terminal_spec.lua | 26 ++++++++++++++++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index 8401343d7a..499716a7a8 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -366,10 +366,10 @@ void terminal_resize(Terminal *term, uint16_t width, uint16_t height) void terminal_enter(void) { buf_T *buf = curbuf; + assert(buf->terminal); // Should only be called when curbuf has a terminal. TerminalState state, *s = &state; memset(s, 0, sizeof(TerminalState)); s->term = buf->terminal; - assert(s->term && "should only be called when curbuf has a terminal"); // Ensure the terminal is properly sized. terminal_resize(s->term, 0, 0); diff --git a/test/functional/terminal/ex_terminal_spec.lua b/test/functional/terminal/ex_terminal_spec.lua index 6f929f17e4..09b4eaa8d5 100644 --- a/test/functional/terminal/ex_terminal_spec.lua +++ b/test/functional/terminal/ex_terminal_spec.lua @@ -13,13 +13,19 @@ describe(':terminal', function() clear() screen = Screen.new(50, 4) screen:attach({rgb=false}) + -- shell-test.c is a fake shell that prints its arguments and exits. nvim('set_option', 'shell', nvim_dir..'/shell-test') nvim('set_option', 'shellcmdflag', 'EXE') - end) + -- Invokes `:terminal {cmd}` using a fake shell (shell-test.c) which prints + -- the {cmd} and exits immediately . + local function terminal_run_fake_shell_cmd(cmd) + execute("terminal "..(cmd and cmd or "")) + end + it('with no argument, acts like termopen()', function() - execute('terminal') + terminal_run_fake_shell_cmd() wait() screen:expect([[ ready $ | @@ -30,7 +36,7 @@ describe(':terminal', function() end) it('executes a given command through the shell', function() - execute('terminal echo hi') + terminal_run_fake_shell_cmd('echo hi') wait() screen:expect([[ ready $ echo hi | @@ -41,7 +47,7 @@ describe(':terminal', function() end) it('allows quotes and slashes', function() - execute([[terminal echo 'hello' \ "world"]]) + terminal_run_fake_shell_cmd([[echo 'hello' \ "world"]]) wait() screen:expect([[ ready $ echo 'hello' \ "world" | @@ -58,4 +64,16 @@ describe(':terminal', function() -- Verify that BufNew actually fired (else the test is invalid). eq('foo', eval('&shell')) end) + + it('ignores writes if the backing stream closes', function() + terminal_run_fake_shell_cmd() + helpers.feed('iiXXXXXXX') + wait() + -- Race: Though the shell exited (and streams were closed by SIGCHLD + -- handler), :terminal cleanup is pending on the main-loop. + -- This write should be ignored (not crash, #5445). + helpers.feed('iiYYYYYYY') + wait() + end) + end) -- cgit From b182f247ecf5d49558aae165ba248e2240d3a5d7 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Wed, 12 Oct 2016 00:04:07 +0200 Subject: eval/term_write(): Skip writes if stream was closed. If the backing stream for a :terminal was closed (e.g. if the shell exits unexpectedly) there may be pending input on the loop which will be processed before the terminal close event (which is queued on the same loop). terminal_send checks term->closed but this does not reflect the state of the underlying streams. The terminal.c module in fact has no knowledge of the streams (this seems intentional: it is abstracted as TerminalOption.write_cb). The SIGCHLD handler (pty_process_unix.c) is executed immediately, and it triggers a stream teardown so Stream.closed=false (TerminalJobData.in.closed). When the pending writes are handled by eval.c:term_write, wstream_write() aborts because it sees the closed Stream. To avoid that, this commit checks Stream.closed in eval:term_write() before writing to the WStream. (As hinted above, we cannot do this in terminal:terminal_send() because that module cannot inspect the underlying streams.) References #5445 https://github.com/neovim/neovim/pull/5445#issuecomment-252529766 --- src/nvim/eval.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 0b1fd6670e..ac8b675834 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -405,7 +405,7 @@ typedef struct { LibuvProcess uv; PtyProcess pty; } proc; - Stream in, out, err; + Stream in, out, err; // Initialized in common_job_start(). Terminal *term; bool stopped; bool exited; @@ -21739,7 +21739,7 @@ static inline TerminalJobData *common_job_init(char **argv, if (!pty) { proc->err = &data->err; } - proc->cb = on_process_exit; + proc->cb = eval_job_process_exit_cb; proc->events = data->events; proc->detach = detach; proc->cwd = cwd; @@ -21923,7 +21923,7 @@ static void on_job_output(Stream *stream, TerminalJobData *data, RBuffer *buf, rbuffer_consumed(buf, count); } -static void on_process_exit(Process *proc, int status, void *d) +static void eval_job_process_exit_cb(Process *proc, int status, void *d) { TerminalJobData *data = d; if (data->term && !data->exited) { @@ -21947,9 +21947,15 @@ static void on_process_exit(Process *proc, int status, void *d) static void term_write(char *buf, size_t size, void *d) { - TerminalJobData *data = d; + TerminalJobData *job = d; + if (job->in.closed) { + // If the backing stream was closed abruptly, there may be write events + // ahead of the terminal close event. Just ignore the writes. + ILOG("write failed: stream is closed"); + return; + } WBuffer *wbuf = wstream_new_buffer(xmemdup(buf, size), size, 1, xfree); - wstream_write(&data->in, wbuf); + wstream_write(&job->in, wbuf); } static void term_resize(uint16_t width, uint16_t height, void *d) -- cgit