refactor!: download packages asynchronously#487
Conversation
nvim-java/nvim-java#487 adds support for async downloading to prevent the UI locking.
s1n7ax
left a comment
s1n7ax
left a comment
There was a problem hiding this comment.
Great motivation — the blocking setup is a real UX problem. A few correctness issues to address before this is mergeable:
lua/pkgm/downloaders/wget.lua — missing return after error
In the failure branch of vim.system's callback, on_finished(nil, err) is called but there's no return, so execution falls through and on_finished(self.dest, nil) is called too. Caller will be invoked twice with conflicting results. Compare with curl.lua which returns correctly.
Errors thrown from libuv callback thread — manager.lua download_package, powershell.lua
vim.system's callback runs on the libuv thread, not the main loop. err_util.throw / notify.error / any vim.api.* call from there is unsafe and can crash. Wrap the failure branches in vim.schedule(function() … end). extract_package correctly uses vim.schedule; the download error paths need the same.
install_all doesn't handle failures
If one install throws (via err_util.throw) inside a callback, the remaining counter never decrements and the final callback() never runs — setup hangs silently. Consider passing an error to on_finished and propagating it to install_all's caller.
Behavior change — vim.lsp.enable('jdtls') now called by plugin
This is a real breaking change for users who already enable jdtls themselves; they'll get double-enable. The PR description flags the breaking change but the README/docs aren't updated. Please document the new contract (either "don't call vim.lsp.enable('jdtls') yourself" or "we no-op if already enabled") and update README.
Type inconsistency — powershell.lua on_finished
Annotated as fun(file_path: string) with no nil/error, while curl.lua/wget.lua use fun(file_path: string|nil, err: string|nil). The dispatcher in manager.lua download_package expects the latter signature. Unify them.
Notify call from callback — manager.lua install
notify.info('Package installed successfully ' .. name) is in the download callback. Same scheduling concern as above.
Untested on Windows is noted in the PR — given the powershell path is the one with the trickiest scheduling, that's the riskiest spot.
s1n7ax
left a comment
s1n7ax
left a comment
There was a problem hiding this comment.
Good direction — blocking setup is a real UX problem. A few correctness issues inline.
panvimdoc was used to auto-generate the vim doc file
@s1n7ax issues fixed, let me know what you think |
2 participants
Problem
Downloading packages is synchronous and blocks the UI. Users cannot type or do anything in Neovim while packages are downloading. Furthermore, because the UI is blocked, users do not know that a package is being downloaded even though nvim-java is supposed to notify the user.
Solution
Download packages asynchronously.
Implementation
At the core, it seems to me that the issue was using the old
vim.fn.systemto execute programs. This PR swaps the oldvim.fn.systemfor the newervim.systemAPI, and makes all executions asynchronous.To make this work, I had to list out all the packages that needed to be installed, instead of installing each package one by one in setup. Then, I pass to the package manager the list of packages that need to be installed. The package manager asynchronously installs each package. When all packages are finished installing, the setup callback is called, and setup can finish.
The main breaking change is that users cannot call
vim.lsp.enable('jdtls')after they callrequire('java').setup()because it no longer blocks, so nvim-java might not be setup before the setup function completes. Because of this, we would need to enable jdtls ourselves after nvim-java is setup. Otherwise, we could create a user autocommand, and have users setup their java tooling in that autocommand.Caution
This PR has not been tested in Windows.