Merge pull request #37249 from zeertzjq/backport

fix(terminal): avoid multiple terminals writing to same buffer (#37219)

src/nvim/api/vim.c

@@ -1018,6 +1018,15 @@ Integer nvim_open_term(Buffer buffer, Dict(open_term) *opts, Error *err)
     return 0;
   }
 
+  if (buf->terminal) {
+    if (terminal_running(buf->terminal)) {
+      api_set_error(err, kErrorTypeException,
+                    "Terminal already connected to buffer %d", buf->handle);
+      return 0;
+    }
+    buf_close_terminal(buf);
+  }
+
   LuaRef cb = LUA_NOREF;
   if (HAS_KEY(opts, open_term, on_input)) {
     cb = opts->on_input;

src/nvim/buffer.c

@@ -480,6 +480,14 @@ static bool can_unload_buffer(buf_T *buf)
   return can_unload;
 }
 
+void buf_close_terminal(buf_T *buf)
+{
+  assert(buf->terminal);
+  buf->b_locked++;
+  terminal_close(&buf->terminal, -1);
+  buf->b_locked--;
+}
+
 /// Close the link to a buffer.
 ///
 /// @param win    If not NULL, set b_last_cursor.
@@ -629,9 +637,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i
   }
 
   if (buf->terminal) {
-    buf->b_locked++;
-    terminal_close(&buf->terminal, -1);
-    buf->b_locked--;
+    buf_close_terminal(buf);
   }
 
   // Always remove the buffer when there is no file name.

src/nvim/eval/funcs.c

@@ -126,6 +126,7 @@
 #include "nvim/strings.h"
 #include "nvim/syntax.h"
 #include "nvim/tag.h"
+#include "nvim/terminal.h"
 #include "nvim/types_defs.h"
 #include "nvim/ui.h"
 #include "nvim/ui_compositor.h"
@@ -4061,6 +4062,14 @@ void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
       shell_free_argv(argv);
       return;
     }
+    if (curbuf->terminal) {
+      if (terminal_running(curbuf->terminal)) {
+        semsg(_("Terminal already connected to buffer %d"), curbuf->handle);
+        shell_free_argv(argv);
+        return;
+      }
+      buf_close_terminal(curbuf);
+    }
     assert(!rpc);
     term_name = "xterm-256color";
     cwd = cwd ? cwd : ".";

src/nvim/terminal.c

@@ -581,7 +581,7 @@ void terminal_close(Terminal **termpp, int status)
 
 #ifdef EXITFREE
   if (entered_free_all_mem) {
-    // If called from close_buffer() inside free_all_mem(), the main loop has
+    // If called from buf_close_terminal() inside free_all_mem(), the main loop has
     // already been freed, so it is not safe to call the close callback here.
     terminal_destroy(termpp);
     return;
@@ -591,7 +591,7 @@ void terminal_close(Terminal **termpp, int status)
   bool only_destroy = false;
 
   if (term->closed) {
-    // If called from close_buffer() after the process has already exited, we
+    // If called from buf_close_terminal() after the process has already exited, we
     // only need to call the close callback to clean up the terminal object.
     only_destroy = true;
   } else {
@@ -608,9 +608,9 @@ void terminal_close(Terminal **termpp, int status)
   buf_T *buf = handle_get_buffer(term->buf_handle);
 
   if (status == -1 || exiting) {
-    // If this was called by close_buffer() (status is -1), or if exiting, we
-    // must inform the buffer the terminal no longer exists so that
-    // close_buffer() won't call this again.
+    // If this was called by buf_close_terminal() (status is -1), or if exiting, we
+    // must inform the buffer the terminal no longer exists so that close_buffer()
+    // won't call buf_close_terminal() again.
     // If inside Terminal mode event handling, setting buf_handle to 0 also
     // informs terminal_enter() to call the close callback before returning.
     term->buf_handle = 0;
@@ -871,7 +871,7 @@ static bool terminal_check_focus(TerminalState *const s)
     set_terminal_winopts(s);
   }
   if (s->term != curbuf->terminal) {
-    // Active terminal buffer changed, flush terminal's cursor state to the UI.
+    // Active terminal changed, flush terminal's cursor state to the UI.
     terminal_focus(s->term, false);
     if (s->close) {
       s->term->destroy = true;

test/functional/terminal/buffer_spec.lua

@@ -882,6 +882,148 @@ describe(':terminal buffer', function()
     ]])
     eq(false, api.nvim_buf_is_valid(term_buf))
   end)
+
+  local function test_open_term_in_buf_with_running_term(env)
+    describe('does not allow', function()
+      it('opening another terminal with jobstart() in same buffer', function()
+        eq(
+          ('Vim:Terminal already connected to buffer %d'):format(env.buf),
+          pcall_err(fn.jobstart, { testprg('tty-test') }, { term = true })
+        )
+        env.screen:expect_unchanged()
+      end)
+
+      it('opening another terminal with nvim_open_term() in same buffer', function()
+        eq(
+          ('Terminal already connected to buffer %d'):format(env.buf),
+          pcall_err(api.nvim_open_term, env.buf, {})
+        )
+        env.screen:expect_unchanged()
+      end)
+    end)
+  end
+
+  describe('with running terminal job', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      fn.jobstart({ testprg('tty-test') }, { term = true })
+      env.screen:expect([[
+        ^tty ready                                                   |
+                                                                    |*5
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_running_term(env)
+  end)
+
+  describe('with open nvim_open_term() channel', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      local chan = api.nvim_open_term(0, {})
+      api.nvim_chan_send(chan, 'TEST')
+      env.screen:expect([[
+        ^TEST                                                        |
+                                                                    |*5
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_running_term(env)
+  end)
+
+  local function test_open_term_in_buf_with_closed_term(env)
+    describe('does not leak memory when', function()
+      describe('opening another terminal with jobstart() in same buffer', function()
+        it('in Normal mode', function()
+          fn.jobstart({ testprg('tty-test') }, { term = true })
+          env.screen:expect([[
+            ^tty ready                                                   |
+                                                                        |*5
+          ]])
+        end)
+
+        it('in Terminal mode', function()
+          feed('i')
+          eq({ blocking = false, mode = 't' }, api.nvim_get_mode())
+          fn.jobstart({ testprg('tty-test') }, { term = true })
+          env.screen:expect([[
+            tty ready                                                   |
+            ^                                                            |
+                                                                        |*3
+            {5:-- TERMINAL --}                                              |
+          ]])
+        end)
+      end)
+
+      describe('opening another terminal with nvim_open_term() in same buffer', function()
+        it('in Normal mode', function()
+          local chan = api.nvim_open_term(env.buf, {})
+          api.nvim_chan_send(chan, 'OTHER')
+          env.screen:expect([[
+            ^OTHER                                                       |
+                                                                        |*5
+          ]])
+        end)
+
+        it('in Terminal mode', function()
+          feed('i')
+          eq({ blocking = false, mode = 't' }, api.nvim_get_mode())
+          local chan = api.nvim_open_term(env.buf, {})
+          api.nvim_chan_send(chan, 'OTHER')
+          env.screen:expect([[
+            OTHER^                                                       |
+                                                                        |*4
+            {5:-- TERMINAL --}                                              |
+          ]])
+        end)
+      end)
+    end)
+  end
+
+  describe('with exited terminal job', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      fn.jobstart({ testprg('shell-test') }, { term = true })
+      env.screen:expect([[
+        ^ready $                                                     |
+        [Process exited 0]                                          |
+                                                                    |*4
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_closed_term(env)
+  end)
+
+  describe('with closed nvim_open_term() channel', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      local chan = api.nvim_open_term(0, {})
+      api.nvim_chan_send(chan, 'TEST')
+      fn.chanclose(chan)
+      env.screen:expect([[
+        ^TEST                                                        |
+        [Terminal closed]                                           |
+                                                                    |*4
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_closed_term(env)
+  end)
 end)
 
 describe('on_lines does not emit out-of-bounds line indexes when', function()