[RFC] Move terminal reedit detection code to do_ecmd() #5445

HarmtH · 2016-10-07T14:37:20Z

Closes issue #4784

justinmk · 2016-10-07T15:13:23Z

Can you add a test?

justinmk · 2016-10-09T10:18:11Z

This is happening because of the autocmd BufReadCmd term:// handler defined in main.c. I am looking to see if there is a way to remove some of these special cases instead.

justinmk · 2016-10-09T12:34:34Z

src/nvim/ex_cmds.c

+      // this is needed for when we are called by do_argfile() and the new
+      // argument index becomes the terminal buffer we are already editing
+      check_arg_idx(curwin);
+      maketitle();


Why is maketitle() needed?

The title shows the index in the argument list (e.g. (2 of 5)), because we can have switched to a new entry (when this function was called from do_argfile()), we need to update the title.

Normally the index in the argument list isn't switched when the buffer is not reloaded or changed.

justinmk · 2016-10-09T14:43:22Z

I was able to reproduce the

src/nvim/event/wstream.c:74: wstream_write: Assertion `!stream->closed' failed.

error (occurring in the builds) locally. Looking at the backtrace now.

Core was generated by `build/bin/nvim -u NONE -i NONE -N --cmd set shortmess+=I'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00002af8cc4e5c37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56      ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  0x00002af8cc4e5c37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00002af8cc4e9028 in __GI_abort () at abort.c:89
#2  0x00002af8cc4debf6 in __assert_fail_base (fmt=0x2af8cc62f3b8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=assertion@entry=0x6caef4 "!stream->closed", file=file@entry=0x6caec9 "../src/nvim/event/wstream.c", line=line@entry=74, 
    function=function@entry=0x6caf04 <__PRETTY_FUNCTION__.13189> "wstream_write") at assert.c:92
#3  0x00002af8cc4deca2 in __GI___assert_fail (assertion=0x6caef4 "!stream->closed", file=0x6caec9 "../src/nvim/event/wstream.c", line=74, 
    function=0x6caf04 <__PRETTY_FUNCTION__.13189> "wstream_write") at assert.c:101
#4  0x00000000004b3901 in wstream_write (stream=0x2af8ccca4878, buffer=0x2af8ccc2d820) at ../src/nvim/event/wstream.c:74
#5  0x00000000004a28c7 in term_write (buf=0x2af8ccc76038 ":", ' ' <repeats 49 times>, size=1, d=0x2af8ccca4700) at ../src/nvim/eval.c:21952
#6  0x000000000063738c in terminal_send (term=0x2af8ccc76000, data=0x2af8ccc76038 ":", ' ' <repeats 49 times>, size=1) at ../src/nvim/terminal.c:509
#7  0x000000000063742f in terminal_send_key (term=0x2af8ccc76000, c=58) at ../src/nvim/terminal.c:525
#8  0x0000000000637216 in terminal_execute (state=0x7fff315b9100, key=58) at ../src/nvim/terminal.c:473
#9  0x000000000061de84 in state_enter (s=0x7fff315b9100) at ../src/nvim/state.c:58
#10 0x0000000000636efa in terminal_enter () at ../src/nvim/terminal.c:393
#11 0x0000000000463872 in edit (cmdchar=105, startln=false, count=1) at ../src/nvim/edit.c:1295
#12 0x000000000055fef4 in normal_finish_command (s=0x7fff315b9260) at ../src/nvim/normal.c:956
#13 0x0000000000560658 in normal_execute (state=0x7fff315b9260, key=58) at ../src/nvim/normal.c:1146
#14 0x000000000061de84 in state_enter (s=0x7fff315b9260) at ../src/nvim/state.c:58
#15 0x000000000055e98f in normal_enter (cmdwin=false, noexmode=false) at ../src/nvim/normal.c:463
#16 0x000000000052277b in main (argc=9, argv=0x7fff315b95b8) at ../src/nvim/main.c:547

HarmtH · 2016-10-09T16:09:57Z

What's happing? Are the execute() functions applied in terminal-mode instead of normal-mode?

justinmk · 2016-10-10T02:34:33Z

The test failure accidentally exposes a bug in our :terminal logic:

In eval.c:common_job_init(), the exit callback is assigned: proc->cb = on_process_exit
- on_process_exit() is what eventually calls terminal_close()
  - terminal_close() sets term->closed, which is needed to avoid trying to use the (closed) stream
When the :term child-process dies, process.c:process_close_event() is scheduled on the process event-loop (which is main-loop, see common_job_init()). It will eventually call on_process_exit() (in eval.c, see above).
But this is too late. If input was pending on the main loop, terminal_execute() will try to write to the closed stream because term->closed won't be set until the event from step 2 is processed.

~~Note: process.c:on_process_exit() has the same problem: even though it's called from SIGCHLD handler in pty_process_unix.c, the cleanup is scheduled on the main-loop.~~ process.c:on_process_exit() is called immediately from chld_handler(), and process_close_handles is scheduled on the process's private loop. This does seem to run before terminal_send is invoked, ~~so maybe we can work with that.~~

References neovim#5445 See neovim#5445 (comment)

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. References neovim#5445 neovim#5445 (comment)

References neovim#5445 See neovim#5445 (comment)

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 neovim#5445 neovim#5445 (comment)

Closes neovim#4784

justinmk · 2016-10-12T01:58:11Z

Merged in #5470. Thank you @HarmtH

References neovim#5445 See neovim#5445 (comment)

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 neovim#5445 neovim#5445 (comment)

Closes neovim#4784

Move terminal reedit detection code to do_ecmd()

c440485

Closes issue neovim#4784

marvim added the RFC label Oct 7, 2016

Adding a test for re-editing terminal buffer

177391f

justinmk reviewed Oct 9, 2016

View reviewed changes

justinmk mentioned this pull request Oct 9, 2016

:UpdateRemotePlugins crashes Neovim when using Common Lisp client #5349

Closed

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 11, 2016

test/terminal: Cover race when :term shell process exits.

bae3fd5

References neovim#5445 See neovim#5445 (comment)

justinmk mentioned this pull request Oct 11, 2016

term_write(): Skip writes if stream was closed. #5463

Merged

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 11, 2016

test/terminal: Cover race when :term shell process exits.

c4a5787

References neovim#5445 See neovim#5445 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 11, 2016

test/terminal: Cover race when :term shell process exits.

4d6cfe6

References neovim#5445 See neovim#5445 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 11, 2016

test/terminal: Cover race when :term shell process exits.

974a719

References neovim#5445 See neovim#5445 (comment)

justinmk pushed a commit to justinmk/neovim that referenced this pull request Oct 12, 2016

terminal: Move re-edit detection to do_ecmd(). neovim#5445

b0179e4

Closes neovim#4784

justinmk mentioned this pull request Oct 12, 2016

Followup #5445 #5470

Merged

justinmk closed this Oct 12, 2016

justinmk removed the RFC label Oct 12, 2016

justinmk added a commit to justinmk/neovim that referenced this pull request Oct 12, 2016

test/terminal: Cover race when :term shell process exits.

ef4676e

References neovim#5445 See neovim#5445 (comment)

justinmk pushed a commit to justinmk/neovim that referenced this pull request Oct 12, 2016

terminal: Move re-edit detection to do_ecmd(). neovim#5445

538255c

Closes neovim#4784

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[RFC] Move terminal reedit detection code to do_ecmd() #5445

[RFC] Move terminal reedit detection code to do_ecmd() #5445

Uh oh!

HarmtH commented Oct 7, 2016

Uh oh!

justinmk commented Oct 7, 2016

Uh oh!

justinmk commented Oct 9, 2016

Uh oh!

justinmk Oct 9, 2016

Uh oh!

HarmtH Oct 9, 2016 •

edited

Loading

Uh oh!

HarmtH Oct 9, 2016 •

edited

Loading

Uh oh!

justinmk commented Oct 9, 2016 •

edited

Loading

Uh oh!

HarmtH commented Oct 9, 2016

Uh oh!

justinmk commented Oct 10, 2016 •

edited

Loading

Uh oh!

justinmk commented Oct 12, 2016

Uh oh!

Uh oh!

Uh oh!

[RFC] Move terminal reedit detection code to do_ecmd() #5445

[RFC] Move terminal reedit detection code to do_ecmd() #5445

Uh oh!

Conversation

HarmtH commented Oct 7, 2016

Uh oh!

justinmk commented Oct 7, 2016

Uh oh!

justinmk commented Oct 9, 2016

Uh oh!

justinmk Oct 9, 2016

Choose a reason for hiding this comment

Uh oh!

HarmtH Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarmtH Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarmtH commented Oct 9, 2016

Uh oh!

justinmk commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Oct 12, 2016

Uh oh!

Uh oh!

HarmtH Oct 9, 2016 •

edited

Loading

HarmtH Oct 9, 2016 •

edited

Loading

justinmk commented Oct 9, 2016 •

edited

Loading

justinmk commented Oct 10, 2016 •

edited

Loading