Reinstalling language parser fails on Windows when a file handle to the old parser exists

[FlippingBinary]

Describe the bug

Treesitter seems to open an exclusive file handle to plugins that are in use, and doesn't release them during reinstallation.

To Reproduce

1. Install/update NeoVim and Treesitter on Windows so they work properly.
2. Open two instances of NeoVim (I launch them in two separate Windows Terminal tabs or windows)
3. Open a file that has a corresponding Treesitter parser in the first instance, e.g. a rust file.
4. In the second instance, run :TSInstall <language>, e.g. :TSInstall rust.
5. Confirm reinstallation with y (this bug only occurs on reinstallation, not initial installation)

Expected behavior

The parser reinstalls successfully.

Output of :checkhealth nvim-treesitter

==============================================================================
nvim-treesitter: require("nvim-treesitter.health").check()

Installation ~
- OK `tree-sitter` found 0.21.0 (1c55abb5308fe3891da545662e5df7ba28ade275) (parser generator, only needed for :TSInstallFromGrammar)
- OK `node` found v20.11.1 (only needed for :TSInstallFromGrammar)
- OK `git` executable found.
- OK `zig` executable found. Selected from { vim.NIL, "cc", "gcc", "clang", "cl", "zig" }
  Version: info: Usage: zig [command] [options]
- OK Neovim was compiled with tree-sitter runtime ABI version 14 (required >=13). Parsers must be compatible with runtime ABI.

OS Info:
{
  machine = "x86_64",
  release = "10.0.19045",
  sysname = "Windows_NT",
  version = "Windows 10 Pro"
} ~

Parser/Features         H L F I J
  - bash                ✓ ✓ ✓ . ✓
  - c                   ✓ ✓ ✓ ✓ ✓
  - css                 ✓ . ✓ ✓ ✓
  - javascript          ✓ ✓ ✓ ✓ ✓
  - json                ✓ ✓ ✓ ✓ .
  - lua                 ✓ ✓ ✓ ✓ ✓
  - markdown            ✓ . ✓ ✓ ✓
  - markdown_inline     ✓ . . . ✓
  - python              ✓ ✓ ✓ ✓ ✓
  - regex               ✓ . . . .
  - rust                ✓ ✓ ✓ ✓ ✓
  - svelte              ✓ ✓ ✓ ✓ ✓
  - toml                ✓ ✓ ✓ ✓ ✓
  - tsx                 ✓ ✓ ✓ ✓ ✓
  - typescript          ✓ ✓ ✓ ✓ ✓
  - vim                 ✓ ✓ ✓ . ✓
  - wgsl                ✓ . ✓ ✓ .
  - yaml                ✓ ✓ ✓ ✓ ✓

  Legend: H[ighlight], L[ocals], F[olds], I[ndents], In[j]ections
         +) multiple parsers found, only one will be used
         x) errors found in the query, try to run :TSUpdate {lang} ~

Output of nvim --version

NVIM v0.9.5
Build type: RelWithDebInfo
LuaJIT 2.1.1703942320
Compilation: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe /MD /Zi /O2 /Ob1  -W3 -wd4311 -wd4146 -DUNIT_TESTING -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE -D_WIN32_WINNT=0x0602 -DMSWIN -DINCLUDE_GENERATED_DECLARATIONS -ID:/a/neovim/neovim/.deps/usr/include/luajit-2.1 -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/build/src/nvim/auto -ID:/a/neovim/neovim/build/include -ID:/a/neovim/neovim/build/cmake.config -ID:/a/neovim/neovim/src -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include -ID:/a/neovim/neovim/.deps/usr/include

   system vimrc file: "$VIM\sysinit.vim"
  fall-back for $VIM: "C:/Program Files (x86)/nvim/share/nvim"

Run :checkhealth for more info

Additional context

I'm using LazyVim, which provides lazy.nvim and nvim-treesitter.

After failing to update a parser, the output of :messages shows this:

[nvim-treesitter] [0/1] Downloading tree-sitter-rust...
[nvim-treesitter] [0/1] Checking out locked revision
[nvim-treesitter] [0/1] Compiling...

nvim-treesitter[rust]: Failed to execute the following command:
{
  cmd = "cmd",
  opts = {
    args = { "/C", "move", "/Y", "parser.so", "C:\\Users\\User\\AppData\\Local\\nvim-data\\lazy\\nvim-treesitter\\parser\\\\rust.so" },
    cwd = "C:\\Users\\User\\AppData\\Local\\nvim-data\\tree-sitter-rust",
    stdio = {
      [2] = <userdata 1>,
      [3] = <userdata 2>
    }
  }
}

When I open another terminal and attempt the command manually:

cd C:\Users\User\AppData\Local\nvim-data\tree-sitter-rust
cmd.exe /C move /Y parser.so C:\Users\User\AppData\Local\nvim-data\lazy\nvim-treesitter\parser\\rust.so

The error message appears like this:

Access is denied.
        0 file(s) moved.

The obvious workaround is to run :TSUpdate or :TSInstall <language> before any affected parsers are loaded. However, that won't prevent the error from occurring, and the error message doesn't guide the user to that workaround.

I most frequently see this error when I open a new instance of NeoVim and see that updated plugins are available, one of which happens to be nvim-treesitter. If I update it, Treesitter may then need to rebuild some of its parsers. The markdown parser always seems to be open, even if there is only one instance of NeoVim with no files open (just the initial start screen with lazy.nvim open). So I see this error often.

Below, I identify four potential solutions:

1. Check the error message output for the phrase Access is denied and print advice to the user suggesting they close every instance of NeoVim and execute nvim -c "TSUpdate" in the terminal. This would help new users avoid the time sink of figuring out which repository is relevant, then searching through open issues and following random troubleshooting advice.
2. Save parsers to a temporary filename such as <language>.so.new when unable to replace the existing parser and check for similarly named files on startup to see if they can be moved to <language>.so at that time. This would act as a kind of two-stage installer where reinstalled/upgraded parsers do not immediately take effect when the old version was in use during installation.
3. Use a file as a mutex to indicate a parser is being built. This file could be named after the parser, such as <language>.so.lock and be used to tell other instances of nvim-treesitter that they need to close any file handle of the affected parser temporarily. The updater could create the file before beginning to build the parser so other instances of nvim-treesitter have time to notice before the new version is actually installed. Then the updater could try to install the updated parser for a limited amount of time with the expectation that the other instances will close the parser when they see the flag, even if they didn't get a chance to before the updater was ready. Meanwhile, the other instances that need the parser can watch for the flag to disappear, indicating it is safe to reopen the new parser.
4. Open parsers without locking them so any process can still modify them, even if they are 'in use'. This would prevent the error altogether and would be ideal, if it doesn't cause other problems.

Edit: Updated some references to 'plugin' when 'parser' was more appropriate.

[clason]

Sorry, but this is too niche to optimize for. (And we can't and won't work around issues with lazy.) You can test the main branch to see if that works better for you, though.

[FlippingBinary]

Okay, but just for clarification: it's not got anything to do with lazy. I mentioned lazy only as an example of one way it gets triggered. I also called the language parsers 'plugins', which may have contributed to some confusion, so I edited that.

I've also discovered a simpler set of reproduction steps:

1. Install/update NeoVim and Treesitter on Windows so they work properly. (this is more of a pre-step)
2. Open a file that has treesitter support in NeoVim, e.g. a Rust file.
3. Close the buffer, but not NeoVim, so nothing needs syntax highlighting.
4. Run :TSInstall <language> where <language> is the name of a language that was highlighted in the file that was open and is now closed, e.g. :TSInstall rust.
5. Confirm reinstallation with y (this bug only occurs on reinstallation, not initial installation)
6. Observe the failure due to the language parser still being locked open.

Even though NeoVim has no open buffers that need the language parser by step 4, the file handle remains open and blocks the reinstall. I have to close NeoVim and reopen it for the reinstall to succeed.

If this is too niche to fix, so be it. I just want to make sure I'm clearly and accurately documenting this problem.

Thank you for your time.

[clason]

That's not something that can be fixed here, though -- the parser use is handled by Neovim itself (and I don't know if tree-sitter itself provides a function for releasing the parser file). We could handle installation failures on Windows better, but

1. none of us use Windows,
2. the master branch is essentially frozen, and
3. the main branch will transition to using tree-sitter build.

Again, can you test the main branch?

[FlippingBinary]

Ahh, thanks for letting me know. I feel a bit sheepish to admit I didn't realize nvim-treesitter isn't involved in the use of the parsers. At this point, it looks like this is a Neovim bug, so I have opened an issue there. The only error I ran into on the main branch that was definitely associated with nvim-treesitter was the use of vim.fs.joinpath, which isn't present in stable versions of Neovim yet. I assume 0.10 will be the minimum required version of Neovim once main is released, but it works fine on a minimal config with 0.10.0-dev-2976+g208852126. Thank you for your help!

[clason]

Yes, nightly is required; I thought the Readme made that clear. I'm not sure there's anything we can do on the neovim side either, to be honest.

[theHamsta]

The problem that @FlippingBinary is mentioning is that we don't really care about garbage collecting parsers. We're well aware that we're leaking the parsers instead of tracking their usage in buffers and then properly dlclose them. This is a problem for Windows because it will not allow to overwrite the old file while it is still in use by libuv.

[FlippingBinary]

@theHamsta Thank you for that info! I found where Neovim calls uv_dlopen, which then calls LoadLibraryExW. That is what appears to be blocking other processes from overwriting the language parser, which explains the error that :TSInstall <existing lanuage> causes if the language parser has been used at all during any open Neovim session.

Obviously, Microsoft is not going to change their architectural decision to enforce exclusive access on loaded dlls any time soon, and nobody wants to impact performance by eliminating caching. However, I may have found a workaround that can be automated in nvim-treesitter during the language parser installation process.

Even though the language parser cannot be replaced on disk while it is loaded, it can be moved.

I confirmed this by changing into the directory and calling cmd /C move /Y rust.so old-rust.so on a language parser that could not be replaced because it was open. I was also able to move it into a subdirectory that I named trash. Even after moving it, it could not be replaced. But that's okay, because there's room for a new parser after the old one is moved out of the way.

So here is what I'm proposing: While reinstalling a language parser on Windows, right before moving the new parser into place, rename the old parser to something that can be easily matched with a wildcard, like <language>.so.<digit>-delete-this where <digit> is 0, or the next available digit (maybe it could try the digits 0 through 9 until it finds one that doesn't fail or just use a big number from a random number generator and call it a day).

Then nvim-treesitter could notify the user to restart Neovim if they want the changes to take effect immediately. Later, it could attempt to delete any stray *-delete-this files out of the parser directory in one wildcard action, possibly during nvim-treesitter's setup function.

If this approach is acceptable, I'd be willing to write up a pull request.

[clason]

Sorry, but that's still too much for a situation we don't see as central. As I said, master is frozen, and we don't accept any such PRs to it. The only (possibly!) acceptable approach I see is adding garbage collection on the Neovim side (but I'm skeptical about that too since it would involve adding significant overhead).

[FlippingBinary]

I'm loving this conversation because I'm learning so much and my proposal is evolving along the way.

@clason My perspective is long-term. I'm willing to contribute a PR to main if that's what it takes to get this problem eventually resolved. Having no solution at all on the timeline is very undesirable. This really does seem like an nvim-treesitter issue to me because it's the unit that is responsible for installing language parsers, and it would take just a minor modification to have it install them using Microsoft's recommendation when on Windows (minus the MOVEFILE_DELAY_UNTIL_REBOOT flag because nvim-treesitter doesn't interact with Win32 APIs directly).

Here is my updated proposal:

1. Rename the old parser to <language>.so.stale-<timestamp>.
2. Install the new parser to <language>.so.
3. Attempt to delete any stale parsers (*.so.stale-*)
4. If the stale parser fails to delete, alert the user that changes will take effect when they restart Neovim.

Similarly, during uninstallation (for completeness):

1. Rename the old parser to <language>.so.stale-<timestamp>.
2. Attempt to delete any stale parsers (*.so.stale-*)

That would be super simple. In many cases (and only on Windows), it would simply reverse the sequence of events with one additional step of moving the file and another informing the user, and I am offering to write the code myself.

Would you please reopen this issue and entertain a PR?