kosayoda / nvim-lightbulb

VSCode 💡 for neovim's built-in LSP.
MIT License
783 stars 27 forks source link

Lightbulb flickering - a race condition when there are multiple LSP clients with code actions #21

Closed Gelio closed 2 years ago

Gelio commented 2 years ago

Hey!

I've been using this plugin for quite some time, but recently (not sure since when) I noticed it does not appear so often. After a bit of debugging I have noticed that the lightbulb disappears moments after it is shown without moving the cursor

https://user-images.githubusercontent.com/889383/137361473-57275395-92ec-45f9-be70-3bd7dbec006a.mp4

You can see the lightbulb appears for a split second only. It does not matter whether there is something in the sign column or not.

My configuration for nvim-lightbulb:

https://github.com/Gelio/ubuntu-dotfiles/blob/0bd99a2124452ea8475a821c3a95ecdd4ff69dda/install/neovim/stowed/.config/nvim/lua/plugins.lua#L469-L481

kosayoda commented 2 years ago

Hello, I took a look at your config but it does not seem trivial for me to reproduce your setup. Can you please make sure you're on the latest master, and if so, either

  1. Try bisecting your config to see what changed
  2. Get a minimal working configuration where the bug happens?
Gelio commented 2 years ago

Sure thing, I'll try to find a reliable minimal reproduction config. I thought it might have been a common bug in the core unrelated to other plugins, but if it's working for you, I'll find the issue on my end.

The interesting thing is that the lightbulb was flickering on my remote VM when I connected to it over SSH. The lightbulb was working fine on my local PC. I have the same plugins and neovim version on both machines.

Anyway, I started by turning off all plugins except for which-key.nvim, nvim-lspconfig, and nvim-lightbulb. The lightbulb was working fine then. I started enabling plugins in chunks. Everything was fine until I enabled null-ls.nvim. Even with all the sources disabled, when null-ls LSP client was active, the lightbulb kept flickering. After doing :LspStop 1 (1 was the ID of the null-ls client), the flickering stopped and the lightbulb stayed in the sign column.

https://user-images.githubusercontent.com/889383/137452089-6ce1744e-d1f0-4dd2-b383-944207a9b3f1.mp4

Then, I prepared this minimal init.lua:

vim.cmd([[packadd packer.nvim]])

vim.o.updatetime = 100

require("packer").startup(function(use)
  use("wbthomason/packer.nvim")

  use({
    "neovim/nvim-lspconfig",
    config = function()
      vim.o.hidden = true
      vim.o.writebackup = false
      vim.o.swapfile = false
      vim.o.cmdheight = 2
      vim.opt.shortmess:append("c")
      vim.o.signcolumn = "auto"

      local null_ls = require("null-ls")
      null_ls.config({
        sources = {
          null_ls.builtins.formatting.prettierd,
        }
      })

      local nvim_lsp = require("lspconfig")
      nvim_lsp["null-ls"].setup({
        capabilities = vim.lsp.protocol.make_client_capabilities()
      })

      nvim_lsp.tsserver.setup({})
    end,
    requires = {
      "jose-elias-alvarez/null-ls.nvim",
      "nvim-lua/plenary.nvim",
    },
  })

  use({
    "kosayoda/nvim-lightbulb",
    config = function()
      vim.cmd([[
        augroup LspLightBulb
          autocmd! CursorHold,CursorHoldI * lua require("nvim-lightbulb").update_lightbulb()
        augroup END
      ]])
    end,
  })
end)

But it turns out... this minimal init.lua is working fine now :facepalm: I probably did something wrong while debugging

I've spent like 1.5h debugging this now and I have no clue what's happening. I will put a bit more time into it over the weekend to try to actual reproduction steps.

Gelio commented 2 years ago

I couldn't put it down in the end, so I spent a bit more time on it. I ended up finding the root cause - a race condition between code actions returned from tsserver and null-ls.nvim (or the lack of them).

TL;DR: IMO the plugin should use vim.lsp.buf_request_all instead of vim.lsp.buf_request in https://github.com/kosayoda/nvim-lightbulb/blob/3c5d42a433fcb73fa9a2e07647ef02288479b284/lua/nvim-lightbulb.lua#L251

Details and some investigation information follows below


In the file I am editing I have 2 code action sources enabled:

  1. tsserver
  2. nvim-lsp-ts-utils registers a code action generator for eslint inside of null-ls.nvim which serves as a LSP client. The relevant code is https://github.com/jose-elias-alvarez/nvim-lsp-ts-utils/blob/efa321ad03fbffeca699bc04ca1a59db0c54d16f/lua/nvim-lsp-ts-utils/null-ls.lua#L181-L199

When I set eslint_enable_code_actions = false in my config for nvim-lsp-ts-utils, the lightbulb was working correctly and didn't disappear. So, when there was only a single code actions provider, it was working fine. It also is in line with what I found earlier that disabling null-ls.nvim LSP client with :LspStop got lightbulb to stick.

I don't have eslint installed in my project, so null-ls.nvim consistently reported there are no code actions. tsserver, on the other hand, reported actions on some lines.

Depending on which LSP client was last in reporting the code actions, the lightbulb either appeared or it did not.

The flickering was caused when tsserver was first to respond with some code actions (lightbulb turned on), but then, a few milliseconds later, null-ls.nvim reported 0 code actions, which hid the lightbulb.

I have added some logs to the code of both nvim-lsp-ts-utils and nvim-lightbulb. Here is the output when the lightbulb stayed on:

update lightbulb {
  code_action_cap_found = true
}
eslint_d output nil
code acitons handler {
  actions = {}
}
code actions handler end
code acitons handler {
  actions = { {
      command = {
        -- some output here
      },
      kind = "quickfix",
      title = "Import 'ObjectSchema' from module \"superstruct/lib/utils\""
    } }
}
code actions handler end

Notice the first code actions handler has 0 actions - that's the one from null-ls.nvim, as it's preceded by printing the eslint output (based on which the list of code actions is built). Then, tsserver responds with its actions. This turns on the lightbulb.

On the other hand, a few seconds later, the order is reversed:

update lightbulb {
  code_action_cap_found = true
}
code acitons handler {
  actions = { {
      command = {
        ...
      },
      kind = "quickfix",
      title = "Import 'ArraySchema' from module \"../json-schema\""
    }, {
      command = {
        ...
      },
      kind = "quickfix",
      title = "Import 'ArraySchema' from module \"./array\""
    } }
}
code actions handler end
eslint_d output nil
code acitons handler {
  actions = {}
}
code actions handler end

and the lightbulb flickers.

After reading nvim-lightbulb source code, looks like vim.lsp.buf_request to get actions https://github.com/kosayoda/nvim-lightbulb/blob/3c5d42a433fcb73fa9a2e07647ef02288479b284/lua/nvim-lightbulb.lua#L251

which invokes the callback multiple times. I experimented with using vim.lsp.buf_request_all which invokes the callback only once, with results from all clients, and it seems to be working consistently now.

Aside from changing the method used, I had to change the handling of parameters inside the callback (https://github.com/kosayoda/nvim-lightbulb/blob/3c5d42a433fcb73fa9a2e07647ef02288479b284/lua/nvim-lightbulb.lua#L144 ) to something like:

    local function code_action_handler(client_responses)
        local has_actions = false
        for _, client_response in pairs(client_responses) do
          if not vim.tbl_isempty(client_response.result) then
            has_actions = true
            break
          end
        end
        -- No available code actions
        if not has_actions then
          -- The code that disabled the lightbulb, and the rest of the original function

I would generally make that into a PR, but I don't know if that's good enough to handle all cases (neovim 0.5 vs 0.5.1 vs nightly), so I would rather just leave my findings and let you make the change that does not break anything :smile:

kosayoda commented 2 years ago

Thank you for the thorough investigation. This does seem to be a bug on my end, and buf_request_all should resolve the issue (only caveat I can think of is slower updates if a client responds slowly, but that's still better than having one response override another).

Feel free to make a PR if you'd like, or I'll get to it as soon as I have spare time.

kosayoda commented 2 years ago

@Gelio I've implemented the fix on the latest master, please let me know if it fixes your issue.

Gelio commented 2 years ago

Thanks! It seems to be working reliably now :+1:

This is pretty much the fix that I was testing out earlier. I thought there would be some extra support required for neovim 0.5 or 0.5.1, but maybe not :slightly_smiling_face:

Thanks for addressing the problem quickly :+1: