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

inlay_hint: "Failed to delete autocmd" error when closing a buffer #24456

Closed
MariaSolOs opened this issue Jul 23, 2023 · 10 comments · Fixed by #24469
Closed

inlay_hint: "Failed to delete autocmd" error when closing a buffer #24456

MariaSolOs opened this issue Jul 23, 2023 · 10 comments · Fixed by #24469
Labels
bug issues reporting wrong behavior lsp
Milestone

Comments

@MariaSolOs
Copy link
Member

Problem

With the latest nightly release, I'm seeing the following error every time I close a buffer:

image

So far I've seen this happen with Lua and Rust files.

Note that this doesn't happen with NVIM v0.10.0-dev-692+g86ce3878d.

Steps to reproduce

The only inlay-hint related setup I have is this pair of autocommands that toggles inlay hints:

    if buf_client.supports_method 'textDocument/inlayHint' then
        local inlay_hints_group = vim.api.nvim_create_augroup('ToggleInlayHints', { clear = false })

        -- Initial inlay hint display.
        local mode = vim.api.nvim_get_mode().mode
        vim.lsp.inlay_hint(bufnr, mode == 'n' or mode == 'v')

        vim.api.nvim_create_autocmd('InsertEnter', {
            group = inlay_hints_group,
            buffer = bufnr,
            callback = function()
                vim.lsp.inlay_hint(bufnr, false)
            end,
        })
        vim.api.nvim_create_autocmd('InsertLeave', {
            group = inlay_hints_group,
            buffer = bufnr,
            callback = function()
                vim.lsp.inlay_hint(bufnr, true)
            end,
        })
    end

The rest of my LSP configuration is here.

Expected behavior

For there to be no error.

Neovim version (nvim -v)

NVIM v0.10.0-dev-709+gce56ad2ba

Vim (not Nvim) behaves the same?

No

Operating system/version

Mac Ventura 13.4.1

Terminal name/version

kitty 0.28.1

$TERM environment variable

xterm-kitty

Installation

bob

@MariaSolOs MariaSolOs added the bug issues reporting wrong behavior label Jul 23, 2023
@MariaSolOs
Copy link
Member Author

This seems to have been caused by #24411. Moreover, I'm not the only one seeing this error, see DNLHC/glance.nvim#57

@zeertzjq zeertzjq added the lsp label Jul 23, 2023
@zeertzjq zeertzjq added this to the 0.10 milestone Jul 23, 2023
@MariaSolOs
Copy link
Member Author

MariaSolOs commented Jul 24, 2023

As extra information: I discovered that this always happens when using bwipeout, but not with bdelete (at least I haven't seen it happen yet). This kind of explains the error since the former removes the buffer-local autocommands.

@justinmk justinmk changed the title "Failed to delete autocmd" error when closing a buffer inlay_hint: "Failed to delete autocmd" error when closing a buffer Jul 24, 2023
@justinmk
Copy link
Member

@catlee

@catlee
Copy link
Contributor

catlee commented Jul 24, 2023

I'm having some problems reproducing this. It doesn't happen all the time, I wonder if there's something racing between bwipeout deleting buffer-local autocommands and the disable() function trying to delete the autocommands as well.

When trying to debug this, I discovered that we are calling nvim_buf_attach each time the inlay hints are enabled. This results in many callbacks being registered on the buffer, and so disable is called multiple times when closing the buffer.

This is a result of my previous PR which removed the enabled field from the internal inlay_hint state table.

I think we need to re-introduce that field, since there doesn't seem to be another way to track when we've registered buffer event handlers, nor is there a way to un-register them?

@justinmk
Copy link
Member

since there doesn't seem to be another way to track when we've registered buffer event handlers, nor is there a way to un-register them?

To unregister a particular handler one would need to remember the id returned by nvim_create_autocmd()

@MariaSolOs
Copy link
Member Author

@catlee thank you for looking into it! If it helps, I could always reproduce it with this command here.

@catlee
Copy link
Contributor

catlee commented Jul 24, 2023

since there doesn't seem to be another way to track when we've registered buffer event handlers, nor is there a way to un-register them?

To unregister a particular handler one would need to remember the id returned by nvim_create_autocmd()

I'm referring to the buffer callbacks here: https://github.com/neovim/neovim/blob/master/runtime/lua/vim/lsp/inlay_hint.lua#L153.

After setting up on_detach / on_reload callbacks, there doesn't seem to be a way to un-register them.

@catlee
Copy link
Contributor

catlee commented Jul 24, 2023

@catlee thank you for looking into it! If it helps, I could always reproduce it with this command here.

Thanks. It only reproduces for me when I have multiple buffers open.

@justinmk
Copy link
Member

:help nvim_buf_detach() mentions :help api-lua-detach:

In-process Lua callbacks can detach by returning `true`. This will detach all
callbacks attached with the same |nvim_buf_attach()| call.

I almost missed that 😅

@catlee
Copy link
Contributor

catlee commented Jul 24, 2023

:help nvim_buf_detach() mentions :help api-lua-detach:

In-process Lua callbacks can detach by returning `true`. This will detach all
callbacks attached with the same |nvim_buf_attach()| call.

I almost missed that 😅

Thanks, that's good to know. I'm not sure how I can use that in this case.

The current code ends up calling nvim_buf_attach every time inlay hints are enabled. If the user toggles hints on/off multiple times without triggering the on_detach or on_reload buffer events, then we end up registering the callbacks multiple times.

justinmk pushed a commit that referenced this issue Aug 1, 2023
…24469

Problem:
"Failed to delete autocmd" error when deleting LspNotify autocmd. #24456

Solution:
Change a few things in the inlay_hint and diagnostic LSP code:
1. Re-introduce the `enabled` flag for the buffer state tables. Previously I was
   relying on the presence of an autocmd id in the state table to track whether
   inlay_hint / diagnostic was enabled for a buffer. There are two reasons why
   this doesn't work well:
  - Each time inlay_hint / diagnostic is enabled, we call `nvim_buf_attach` on
    the buffer, resulting in multiple `on_reload` or `on_detach` callbacks being
    registered.
  - Commands like `bwipeout` delete buffer local autocmds, sometimes before our
    `on_detach` callbacks have a chance to delete them first. This causes the
  - Use module local enabled state for diagnostic as well. bwipeout can race
    with on_detach callbacks for deleting autocmds. Error referenced in #24456.
2. Change the `LspDetach` autocmd to run each time (i.e., remove the `once`
   flag). Since we're only registering autocmds once per buffer now, we
   need to make sure that we set the enabled flag properly each time the LSP
   client detaches from the buffer.
  - Remove `once` from the LspDetach autocmds for inlay_hint and diagnostic.
    We only set up the autocmd once now. Gets removed when buffer is deleted.
3. Have the `LspNotify` handler also refresh the inlay_hint / diagnostics when
   receiving the `textDocument/didOpen` event. Before this point, the LSP
   backend doesn't have the contents of the buffer, so can't provide inlay hints
   or diagnostics.

Downsides of this approach:
* When inlay_hint / diagnostics are disabled on a buffer, it will continue to
  receive `LspNotify` events for that buffer. The callback exits early since the
  `enabled` flag is false.

Alternatives:
* Can we wrap the call to `nvim_del_autocmd` in `pcall` to swallow any errors
  resulting from trying to delete the autocmd?

Fixes #24456

Helped-by: Maria José Solano <majosolano99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues reporting wrong behavior lsp
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants