Merge pull request #17687 from zeertzjq/fix-channel-consistency

fix(channel): fix channel consistency

src/nvim/api/vim.c

@@ -1140,6 +1140,7 @@ Integer nvim_open_term(Buffer buffer, DictionaryOf(LuaRef) opts, Error *err)
   TerminalOptions topts;
   Channel *chan = channel_alloc(kChannelStreamInternal);
   chan->stream.internal.cb = cb;
+  chan->stream.internal.closed = false;
   topts.data = chan;
   // NB: overridden in terminal_check_size if a window is already
   // displaying the buffer

src/nvim/channel.c

@@ -138,8 +138,14 @@ bool channel_close(uint64_t id, ChannelPart part, const char **error)
       *error = (const char *)e_invstream;
       return false;
     }
-    api_free_luaref(chan->stream.internal.cb);
-    chan->stream.internal.cb = LUA_NOREF;
+    if (chan->term) {
+      api_free_luaref(chan->stream.internal.cb);
+      chan->stream.internal.cb = LUA_NOREF;
+      chan->stream.internal.closed = true;
+      terminal_close(chan->term, 0);
+    } else {
+      channel_decref(chan);
+    }
     break;
 
   default:
@@ -536,7 +542,11 @@ size_t channel_send(uint64_t id, char *data, size_t len, bool data_owned, const
   }
 
   if (chan->streamtype == kChannelStreamInternal) {
-    if (!chan->term) {
+    if (chan->is_rpc) {
+      *error = _("Can't send raw data to rpc channel");
+      goto retfree;
+    }
+    if (!chan->term || chan->stream.internal.closed) {
       *error = _("Can't send data to closed stream");
       goto retfree;
     }

src/nvim/channel.h

@@ -44,6 +44,7 @@ typedef struct {
 
 typedef struct {
   LuaRef cb;
+  bool closed;
 } InternalState;
 
 typedef struct {

src/nvim/terminal.c

@@ -317,10 +317,14 @@ void terminal_close(Terminal *term, int status)
       term->opts.close_cb(term->opts.data);
     }
   } else if (!only_destroy) {
-    // This was called by channel_process_exit_cb() not in process_teardown().
+    // Associated channel has been closed and the editor is not exiting.
     // Do not call the close callback now. Wait for the user to press a key.
     char msg[sizeof("\r\n[Process exited ]") + NUMBUFLEN];
-    snprintf(msg, sizeof msg, "\r\n[Process exited %d]", status);
+    if (((Channel *)term->opts.data)->streamtype == kChannelStreamInternal) {
+      snprintf(msg, sizeof msg, "\r\n[Terminal closed]");
+    } else {
+      snprintf(msg, sizeof msg, "\r\n[Process exited %d]", status);
+    }
     terminal_receive(term, msg, strlen(msg));
   }
 

test/functional/core/channels_spec.lua

@@ -10,6 +10,8 @@ local nvim_prog = helpers.nvim_prog
 local is_os = helpers.is_os
 local retry = helpers.retry
 local expect_twostreams = helpers.expect_twostreams
+local assert_alive = helpers.assert_alive
+local pcall_err = helpers.pcall_err
 
 describe('channels', function()
   local init = [[
@@ -314,3 +316,22 @@ describe('channels', function()
     eq({"notification", "exit", {id, 1, {''}}}, next_msg())
   end)
 end)
+
+describe('loopback', function()
+  before_each(function()
+    clear()
+    command("let chan = sockconnect('pipe', v:servername, {'rpc': v:true})")
+  end)
+
+  it('does not crash when sending raw data', function()
+    eq("Vim(call):Can't send raw data to rpc channel",
+       pcall_err(command, "call chansend(chan, 'test')"))
+    assert_alive()
+  end)
+
+  it('are released when closed', function()
+    local chans = eval('len(nvim_list_chans())')
+    command('call chanclose(chan)')
+    eq(chans - 1, eval('len(nvim_list_chans())'))
+  end)
+end)

test/functional/terminal/channel_spec.lua

@@ -1,45 +1,94 @@
 local helpers = require('test.functional.helpers')(after_each)
+local Screen = require('test.functional.ui.screen')
 local clear = helpers.clear
 local eq = helpers.eq
+local eval = helpers.eval
 local command = helpers.command
 local pcall_err = helpers.pcall_err
 local feed = helpers.feed
-local sleep = helpers.sleep
 local poke_eventloop = helpers.poke_eventloop
 
-describe('associated channel is closed and later freed for terminal', function()
-  before_each(clear)
+describe('terminal channel is closed and later released if', function()
+  local screen
+
+  before_each(function()
+    clear()
+    screen = Screen.new()
+    screen:attach()
+  end)
 
   it('opened by nvim_open_term() and deleted by :bdelete!', function()
     command([[let id = nvim_open_term(0, {})]])
-    -- channel hasn't been freed yet
-    eq("Vim(call):Can't send data to closed stream", pcall_err(command, [[bdelete! | call chansend(id, 'test')]]))
-    -- channel has been freed after one main loop iteration
-    eq("Vim(call):E900: Invalid channel id", pcall_err(command, [[call chansend(id, 'test')]]))
+    local chans = eval('len(nvim_list_chans())')
+    -- channel hasn't been released yet
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[bdelete! | call chansend(id, 'test')]]))
+    -- channel has been released after one main loop iteration
+    eq(chans - 1, eval('len(nvim_list_chans())'))
   end)
 
-  it('opened by termopen(), exited, and deleted by pressing a key', function()
-    command([[let id = termopen('echo')]])
-    sleep(500)
-    -- process has exited
-    eq("Vim(call):Can't send data to closed stream", pcall_err(command, [[call chansend(id, 'test')]]))
+  it('opened by nvim_open_term(), closed by chanclose(), and deleted by pressing a key', function()
+    command('let id = nvim_open_term(0, {})')
+    local chans = eval('len(nvim_list_chans())')
+    -- channel has been closed but not released
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[call chanclose(id) | call chansend(id, 'test')]]))
+    screen:expect({any='%[Terminal closed]'})
+    eq(chans, eval('len(nvim_list_chans())'))
     -- delete terminal
     feed('i<CR>')
     -- need to first process input
     poke_eventloop()
-    -- channel has been freed after another main loop iteration
-    eq("Vim(call):E900: Invalid channel id", pcall_err(command, [[call chansend(id, 'test')]]))
+    -- channel has been released after another main loop iteration
+    eq(chans - 1, eval('len(nvim_list_chans())'))
+  end)
+
+  it('opened by nvim_open_term(), closed by chanclose(), and deleted by :bdelete', function()
+    command('let id = nvim_open_term(0, {})')
+    local chans = eval('len(nvim_list_chans())')
+    -- channel has been closed but not released
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[call chanclose(id) | call chansend(id, 'test')]]))
+    screen:expect({any='%[Terminal closed]'})
+    eq(chans, eval('len(nvim_list_chans())'))
+    -- channel still hasn't been released yet
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[bdelete | call chansend(id, 'test')]]))
+    -- channel has been released after one main loop iteration
+    eq(chans - 1, eval('len(nvim_list_chans())'))
+  end)
+
+  it('opened by termopen(), exited, and deleted by pressing a key', function()
+    command([[let id = termopen('echo')]])
+    local chans = eval('len(nvim_list_chans())')
+    -- wait for process to exit
+    screen:expect({any='%[Process exited 0%]'})
+    -- process has exited but channel has't been released
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[call chansend(id, 'test')]]))
+    eq(chans, eval('len(nvim_list_chans())'))
+    -- delete terminal
+    feed('i<CR>')
+    -- need to first process input
+    poke_eventloop()
+    -- channel has been released after another main loop iteration
+    eq(chans - 1, eval('len(nvim_list_chans())'))
   end)
 
   -- This indirectly covers #16264
   it('opened by termopen(), exited, and deleted by :bdelete', function()
     command([[let id = termopen('echo')]])
-    sleep(500)
-    -- process has exited
-    eq("Vim(call):Can't send data to closed stream", pcall_err(command, [[call chansend(id, 'test')]]))
-    -- channel hasn't been freed yet
-    eq("Vim(call):Can't send data to closed stream", pcall_err(command, [[bdelete | call chansend(id, 'test')]]))
-    -- channel has been freed after one main loop iteration
-    eq("Vim(call):E900: Invalid channel id", pcall_err(command, [[call chansend(id, 'test')]]))
+    local chans = eval('len(nvim_list_chans())')
+    -- wait for process to exit
+    screen:expect({any='%[Process exited 0%]'})
+    -- process has exited but channel hasn't been released
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[call chansend(id, 'test')]]))
+    eq(chans, eval('len(nvim_list_chans())'))
+    -- channel still hasn't been released yet
+    eq("Vim(call):Can't send data to closed stream",
+       pcall_err(command, [[bdelete | call chansend(id, 'test')]]))
+    -- channel has been released after one main loop iteration
+    eq(chans - 1, eval('len(nvim_list_chans())'))
   end)
 end)