Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

screenstring() doesn't work for Command line in child Neovim process #21886

Open
echasnovski opened this issue Jan 18, 2023 · 10 comments
Open

screenstring() doesn't work for Command line in child Neovim process #21886

echasnovski opened this issue Jan 18, 2023 · 10 comments
Labels
bug-regression wrong behavior that was introduced in a previous commit (please bisect) messages UI messages, log messages startup Nvim startup sequence (`:h startup`)
Milestone

Comments

@echasnovski
Copy link
Member

Describe the bug

The screenstring() executed inside child Neovim process through RPC returns incorrect results for Command line area (omits its content while working correctly for showcmd area).

Judging by my GitHub Actions, it worked on commit 6134c1e but stopped working on f6929ea.

From my work on #19020 I remember that this Command line area is treated somewhat differently from the rest of the window. This is based on these lines but I am not sure how it could have gotten affected in the previous couple of days.

@bfredl, sorry to mention you here to get your attention, but it seems you might have an idea of what is going on here. This breaks screenshot tests so it would be bad for this issue to get lost among others.

Steps to reproduce

  • Create 'init-child-screenstring.lua' with the following content:

    init-child-screenstring.lua
    _G.new_child = function(nvim_executable)
      -- Create new child process
      local stdin = vim.loop.new_pipe()
      local stdout = vim.loop.new_pipe()
      local stderr = vim.loop.new_pipe()
    
      local address = vim.fn.tempname()
    
      vim.loop.spawn(nvim_executable or 'nvim', {
        stdio = { stdin, stdout, stderr },
        args = { '--clean', '--listen', address },
      }, function() end)
    
      -- Wait for Neovim to open
      vim.loop.sleep(200)
    
      -- Connect
      local channel = vim.fn.sockconnect('pipe', address, { rpc = true })
    
      -- Follow advice from https://github.com/neovim/neovim/issues/21630
      stdin:close()
      stdout:close()
      stderr:close()
    
      -- Create helper methods for easier demonstration
      local child = { channel = channel }
    
      -- Type keys (for initializing Visual mode)
      child.type_keys = function(keys) vim.rpcrequest(channel, 'nvim_input', keys) end
    
      -- Echo message in Command line area
      child.echo = function(msg) vim.rpcrequest(channel, 'nvim_echo', { { msg } }, false, {}) end
    
      -- Call `screenstring()` inside child process
      child.screenstring = function(row, col)
        return vim.rpcrequest(channel, 'nvim_exec_lua', 'return vim.fn.screenstring(...)', { row, col })
      end
    
      -- Main demo: print child process's last line (Command line with defaults)
      child.print_last_line = function()
        local n_lines = vim.rpcrequest(channel, 'nvim_get_option', 'lines')
        local n_cols = vim.rpcrequest(channel, 'nvim_get_option', 'columns')
        local last_line = {}
        for i = 1, n_cols do
          table.insert(last_line, child.screenstring(n_lines, i))
        end
    
        print(vim.inspect(table.concat(last_line)))
      end
    
      return child
    end

    It is basically a creation-connection to full child process (following advice from Can't receive an answer from RPC request to Neovim created as child process #21630) and then a couple helper wrappers to get information from child process through RPC requests.

  • Assuming nvim_nightly_g737 is the executable for latest nightly, run nvim_nightly_g737 -u init-child-screenstring.lua.

  • :lua child = new_child('nvim_nightly_g737'). This should create and connect to child process running latest nightly.

  • Execute one of the following:

    • :lua child.type_keys('V') - this should initialize Visual linewise mode. When executed in non-child process it is followed by showing -- VISUAL LINE -- in Command line.
    • :lua child.echo('Hello world!') - this should show the message in Command line.
  • Execute :lua child.print_last_line(). For both of versions of previous step it prints content of Command line but without messages at the start.

Expected behavior

Prints actual content of Command line:

  • The one starting with -- VISUAL LINE -- if child.type_keys('V') was executed.
  • The one starting with Hello world! if child.echo('Hello world!') was executed.

To check the expected content, initiate child process with :lua child = new_child('nvim') (assuming nvim is an executable for commit earlier than 6134c1e (like 0.8.2).

Neovim version (nvim -v)

NVIM v0.9.0-dev-737+g0aae7f386

Vim (not Nvim) behaves the same?

No (doesn't have this functionality)

Operating system/version

EndeavourOS Linux x86_64 (6.1.6-arch1-1)

Terminal name/version

st-0.9

$TERM environment variable

st-256color

Installation

appimage

@echasnovski echasnovski added the bug issues reporting wrong behavior label Jan 18, 2023
@zeertzjq
Copy link
Member

zeertzjq commented Jan 19, 2023

This happens because there is no UI (#21831), and messages are printed on stdout/stderr.
vim.rpcrequest(child.channel, 'nvim_ui_attach', 80, 24, {}) may be a solution, but it currently causes a crash. I've created a PR #21892 to fix the crash.

@zeertzjq zeertzjq added bug-regression wrong behavior that was introduced in a previous commit (please bisect) startup Nvim startup sequence (`:h startup`) ui tui and removed bug issues reporting wrong behavior ui tui labels Jan 19, 2023
@echasnovski
Copy link
Member Author

Thanks for clarification!

Judging by the changed definition of use_builtin_ui, I hoped that removing stdout:close() and stderr:close() would make it work, but it doesn't :(

echasnovski added a commit to echasnovski/mini.nvim that referenced this issue Jan 19, 2023
Details:
- At the moment 'mini.test' on it fails to correctly compute child
  process screenshot for Command line area. See neovim/neovim#21886.
shortcuts added a commit to shortcuts/no-neck-pain.nvim that referenced this issue Jan 19, 2023
@zeertzjq
Copy link
Member

zeertzjq commented Jan 21, 2023

You can also spawn another process to attach a UI:

--- a/init-child-screenstring.lua
+++ b/init-child-screenstring.lua
@@ -14,6 +14,10 @@ _G.new_child = function(nvim_executable)
   -- Wait for Neovim to open
   vim.loop.sleep(200)
 
+  vim.loop.spawn(nvim_executable or 'nvim', {
+    args = { '--remote-ui', '--server', address },
+  }, function() end)
+
   -- Connect
   local channel = vim.fn.sockconnect('pipe', address, { rpc = true })
 

@zeertzjq zeertzjq added the has:workaround issue is not fixed but can be circumvented until then label Jan 21, 2023
@echasnovski
Copy link
Member Author

You can also spawn another process to attach a UI:

Thanks!

This indeed solves my use case but only locally. I can't get this working in GitHub Actions... I added check to see if UI is actually attached, and it seems not the case.

Here is essentially what I added:

--- a/init-child-screenstring.lua
+++ b/init-child-screenstring.lua
@@ -22,6 +22,29 @@ _G.new_child = function(nvim_executable)
   stdout:close()
   stderr:close()

+  -- Attach remote UI in case of Neovim>=0.9
+  local has_09 = vim.rpcrequest(channel, 'nvim_exec_lua', [[return vim.fn.has('nvim-0.9') == 1]], {})
+  if has_09 then
+    vim.loop.spawn(nvim_executable or 'nvim', {
+      args = { '--remote-ui', '--server', address },
+    }, function() end)
+
+    -- Ensure that there is an attached UI
+    local step = 10
+    local is_success, uis = false, nil
+    local i, max_tries = 0, 500
+    repeat
+      i = i + 1
+      vim.loop.sleep(step)
+      is_success, uis = pcall(vim.rpcrequest, channel, 'nvim_list_uis')
+      if type(uis) == 'table' then is_success = #uis > 0 end
+    until is_success or i >= max_tries
+
+    if not is_success then
+      error('Can not connect remote UI')
+    end
+  end
+
   -- Create helper methods for easier demonstration
   local child = { channel = channel }

It works locally, but not on CI (throws error "Can not connect remote UI").

Do you have any idea why this might be the case?

@echasnovski
Copy link
Member Author

Also, Is it reasonable to ask this to be added to 0.9 milestone? So as to have a more stable solution than this workaround. Having echoed and other messages not appear in this situation while everything else does seems a bit strange.

@zeertzjq zeertzjq added this to the 0.9 milestone Jan 21, 2023
@zeertzjq zeertzjq removed the has:workaround issue is not fixed but can be circumvented until then label Jan 21, 2023
@zeertzjq
Copy link
Member

zeertzjq commented Jan 29, 2023

Will using termopen() instead be a solution for you?

--- a/init-child-screenstring.lua
+++ b/init-child-screenstring.lua
@@ -1,15 +1,8 @@
 _G.new_child = function(nvim_executable)
-  -- Create new child process
-  local stdin = vim.loop.new_pipe()
-  local stdout = vim.loop.new_pipe()
-  local stderr = vim.loop.new_pipe()
-
   local address = vim.fn.tempname()
 
-  vim.loop.spawn(nvim_executable or 'nvim', {
-    stdio = { stdin, stdout, stderr },
-    args = { '--clean', '--listen', address },
-  }, function() end)
+  -- Create new child process
+  vim.fn.termopen({ nvim_executable or 'nvim', '--clean', '--listen', address })
 
   -- Wait for Neovim to open
   vim.loop.sleep(200)
@@ -17,11 +10,6 @@ _G.new_child = function(nvim_executable)
   -- Connect
   local channel = vim.fn.sockconnect('pipe', address, { rpc = true })
 
-  -- Follow advice from https://github.com/neovim/neovim/issues/21630
-  stdin:close()
-  stdout:close()
-  stderr:close()
-
   -- Create helper methods for easier demonstration
   local child = { channel = channel }
 

@echasnovski
Copy link
Member Author

Will using termopen() instead be a solution for you?

This is an interesting approach, didn't think about it. Thank you @zeertzjq for taking your time!

I've just tested it in my main use case of 'mini.test' testing framework. Here are an original (current) code and commit with changes. Important note is that the child process opens and closes on every test case, which number is currently at 1825. So a lot.

The results are... mixed:

  • It kind of works locally with the following caveats:
    • Main problem is that after some amount of executed test (can't say exact number, around 300-350 cases) it stops with errors like ERR 2023-01-29T13:00:27.593 nvim.42386.0 open_log_file:210: failed to open $NVIM_LOG_FILE (Too many open files) and ERR 2023-01-29T13:00:27.593 nvim.42386.0 set_duplicating_descriptor:376: Failed to dup descriptor 1021: Too many open files. So I am not able to execute all tests at once. This happens both when child processes are opened from both "normal" and headless Neovim instances.
    • When executed inside "normal" Neovim instance, it leads to TermOpen events being triggered (with all their autocommands). Not a huge deal, but not good.
    • I somehow disables all highlighting in opened buffers. Calling :edit inside every one fixes it, but not good.
  • Inside GitHub Actions CI it mostly seems to work. It runs inside headless Neovim instance, of course.

So, all in all, as a very hacky workaround this might be made working. However, it doesn't seem to be a good idea to ignore original issue of screenstring() returning incorrect results. Especially with #22018 coming to light.

@zeertzjq
Copy link
Member

zeertzjq commented Jan 29, 2023

Then you can try jobstart() with pty = true, which is similar to termopen(), and defaults to 80x24, but doesn't trigger TermOpen events.

--- a/init-child-screenstring.lua
+++ b/init-child-screenstring.lua
@@ -1,15 +1,8 @@
 _G.new_child = function(nvim_executable)
-  -- Create new child process
-  local stdin = vim.loop.new_pipe()
-  local stdout = vim.loop.new_pipe()
-  local stderr = vim.loop.new_pipe()
-
   local address = vim.fn.tempname()
 
-  vim.loop.spawn(nvim_executable or 'nvim', {
-    stdio = { stdin, stdout, stderr },
-    args = { '--clean', '--listen', address },
-  }, function() end)
+  -- Create new child process
+  vim.fn.jobstart({ nvim_executable or 'nvim', '--clean', '--listen', address }, { pty = true })
 
   -- Wait for Neovim to open
   vim.loop.sleep(200)
@@ -17,11 +10,6 @@ _G.new_child = function(nvim_executable)
   -- Connect
   local channel = vim.fn.sockconnect('pipe', address, { rpc = true })
 
-  -- Follow advice from https://github.com/neovim/neovim/issues/21630
-  stdin:close()
-  stdout:close()
-  stderr:close()
-
   -- Create helper methods for easier demonstration
   local child = { channel = channel }
 

@echasnovski
Copy link
Member Author

Then you can try jobstart() with pty = true, which is similar to termopen(), and defaults to 80x24, but doesn't trigger TermOpen events.

Yes, this is better. Issues with TermOpen and highlighting seems to be gone now. Thanks!

I still can't test locally many cases in a row. It throws these kind of errors after around 300 (maybe 256-ish?) times of open-close child process (on both Neovim 0.8.2 and NVIM v0.9.0-dev-825+g843c9025a):

ERR 2023-01-29T17:11:27.952 nvim.89004.0 open_log_file:210: failed to open $NVIM_LOG_FILE (Too many open files):
ERR 2023-01-29T17:11:27.952 nvim.89004.0 set_duplicating_descriptor:376: Failed to dup descriptor 1021: Too many open files
Vim:E903: Process failed to start: too many open files: "/tmp/.mount_nvimrUXvs0/usr/bin/nvim"

For reference, here is the commit.

I remember going down a rabbit whole when writing 'mini.test' framework that vim.loop.spawn() was the only solution that I could come up to work without issues.

@echasnovski
Copy link
Member Author

Then you can try jobstart() with pty = true, which is similar to termopen(), and defaults to 80x24, but doesn't trigger TermOpen events.

--- a/init-child-screenstring.lua
+++ b/init-child-screenstring.lua
@@ -1,15 +1,8 @@
 _G.new_child = function(nvim_executable)
-  -- Create new child process
-  local stdin = vim.loop.new_pipe()
-  local stdout = vim.loop.new_pipe()
-  local stderr = vim.loop.new_pipe()
-
   local address = vim.fn.tempname()
 
-  vim.loop.spawn(nvim_executable or 'nvim', {
-    stdio = { stdin, stdout, stderr },
-    args = { '--clean', '--listen', address },
-  }, function() end)
+  -- Create new child process
+  vim.fn.jobstart({ nvim_executable or 'nvim', '--clean', '--listen', address }, { pty = true })
 
   -- Wait for Neovim to open
   vim.loop.sleep(200)
@@ -17,11 +10,6 @@ _G.new_child = function(nvim_executable)
   -- Connect
   local channel = vim.fn.sockconnect('pipe', address, { rpc = true })
 
-  -- Follow advice from https://github.com/neovim/neovim/issues/21630
-  stdin:close()
-  stdout:close()
-  stderr:close()
-
   -- Create helper methods for easier demonstration
   local child = { channel = channel }
 

I've been testing this solution for some time now and it does seem to work and fix this issue.

I don't have problem with this being close, but it still seems to not be a good behavior to have in an otherwise valid child process.

@bfredl bfredl modified the milestones: 0.9, 0.10 Apr 7, 2023
@zeertzjq zeertzjq modified the milestones: 0.10, 0.11 Mar 21, 2024
@zeertzjq zeertzjq added the messages UI messages, log messages label Mar 21, 2024
@zeertzjq zeertzjq modified the milestones: 0.11, 0.10 Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-regression wrong behavior that was introduced in a previous commit (please bisect) messages UI messages, log messages startup Nvim startup sequence (`:h startup`)
Projects
None yet
Development

No branches or pull requests

3 participants