Skip to content

feat: nio async library#228

Merged
rcarriga merged 6 commits into
masterfrom
feat/nio-async-library
Apr 15, 2023
Merged

feat: nio async library#228
rcarriga merged 6 commits into
masterfrom
feat/nio-async-library

Conversation

@rcarriga
Copy link
Copy Markdown
Collaborator

@rcarriga rcarriga commented Apr 2, 2023

Neotest is migrating away from using plenary's async library, and replacing it with a new library called nio. There are several reasons for this change.

Reasoning

Some major ones are:

  • Documentation: There is no vimdoc documentation and minimal documentation in the README.
  • Typing: I believe providing type hints and docstrings make a huge difference when using a
    library. Plenary has very few of either.
  • Consistency: The API has several inconsistencies that mean I regularly have to
    check source code for how to use it (e.g. some control objects use regular .
    syntax such as mpsc whereas others use : syntax such as Semaphore).
  • Error handling: Handling errors in async functions requires using a custom apcall

This is not to say the plenary library has not been great and I absolutely appreaciate all the effort that went into it but the library is now seemingly no longer actively maintained (e.g. nvim-lua/plenary.nvim#293 and nvim-lua/plenary.nvim#298) and I believe there are significant enough changes to be made to justify packaging a new library.

The new library has a simple goal: providing common asynchronous primitives and asynchronous APIs for Neovim's core. It has been heavily influenced by the previous work for async IO in neovim including plenary and others. There are plans for Neovim to embed an async library within core which will provide a barebones core for libraries to share for compatibility (see neovim/neovim#19624). Nio aims to maintain compatibility with this library.

Features of nio include:

  • Task objects which track running async functions
  • Handling results and errors of tasks along with cancelling.
  • Helper functions for coordinating tasks, including gathering results from a list of tasks and getting the result of the first task to complete (and cancelling the others).
  • Async versions of many libuv functions, pretty much the same way plenary does it with the added bonus of all of the functions being typed with emmylua annotations so users will have full type inference.
  • Creating a fully documented and typed LSP client interface which is generated by a script based off of the LSP spec meta model and then implemented very simply with some metatables.
  • Async primitives (futures, queues, events and semaphores) which are often useful in async code, again based off of Python's asyncio modules.

What does this mean for adapters?

I've been using the library embedded in nvim-dap-ui for a while now but neotest provides a better use case as the async usages are more complex and diverse. Right now I'll be leaving it embedded in neotest, with it being released as a separate plugin in future.

I've checked the source code all the adapters mentioned here (gotten from the neotest README) and most of them won't see any user facing issues as they are using neotest.async which has been migrated to point to nio. All usage of fn, api and uv will work as before.

Those adapters using require("plenary.async.tests") will have broken tests, which can be fixed by switching to require("nio").tests.

neotest-jest and neotest-dotnet do use the control module which which has changed significantly. I will open PRs to make changes to use the new primitives.

I'm planning on merging these changes relatively soon so please let me know of any issues but hopefully this will be a painless switch 😄

Comment thread lua/neotest/client/strategies/dap/init.lua
@rcarriga rcarriga mentioned this pull request Apr 10, 2023
@rcarriga
Copy link
Copy Markdown
Collaborator Author

neotest-dotnet: @Issafalcon
neotest-deno: @MarkEmmons
neotest-jest: @haydenmeade
neotest-elixir: @jfpedroza
neotest-vitest: @marilari88
neotest-haskell: @mrcjkb
neotest-go: @sergii4 @akinsho
neotest-phpunit: @olimorris
neotest-rspec: @olimorris
neotest-rust: @rouge8
neotest-testthat: @shunsambongi
neotest-dart: @sidlatau
neotest-scala: @stevanmilic
neotest-pest: @theutz

rcarriga added a commit to rcarriga/neotest-dotnet that referenced this pull request Apr 10, 2023
Just a small change to adapt to the new async library in neotest.

Depends on nvim-neotest/neotest#228
rcarriga added a commit to rcarriga/neotest-jest that referenced this pull request Apr 10, 2023
Just some small changes to adapt to the new async library in neotest.

Depends on nvim-neotest/neotest#228
rcarriga added a commit to rcarriga/neotest-dotnet that referenced this pull request Apr 10, 2023
Just a small change to adapt to the new async library in neotest.

Depends on nvim-neotest/neotest#228
@mrcjkb
Copy link
Copy Markdown
Contributor

mrcjkb commented Apr 10, 2023

Had to replace a neotest.async require with nio, but otherwise all is good from my side.

olimorris added a commit to olimorris/neotest-rspec that referenced this pull request Apr 10, 2023
olimorris added a commit to olimorris/neotest-phpunit that referenced this pull request Apr 10, 2023
@olimorris
Copy link
Copy Markdown
Contributor

Completely agree with the rationale. I've found the documentation for plenary to be subpar for a while now (ChatGPT to the rescue).

I updated my adapters with a conditional for nio.

I noticed that my overseer consumer is broken though (cc: @stevearc)

@stevearc
Copy link
Copy Markdown
Contributor

I have a PR up for overseer that uses the new library and generally works, but it spits out this error:

[ERROR] Task Neotest dispatch callback on_output: ...vearc/.local/share/nvim/lazy/neotest/lua/nio/control.lua:48: attempt to call local 'callback' (a nil value)

I've tried fiddling with the implementation and passing functions around, but I can't seem to find where this is coming from. This highlights my main issue with writing async code in most languages: stack traces lose information and it becomes a lot harder to debug.

@rcarriga Do you have any tips for figuring out where this control.wait is getting called from?

@rcarriga
Copy link
Copy Markdown
Collaborator Author

Thanks for the testing and updating everyone! I'd forgotten to check overseer, but great to see it updated too.

I've tried fiddling with the implementation and passing functions around, but I can't seem to find where this is coming from. This highlights my main issue with writing async code in most languages: stack traces lose information and it becomes a lot harder to debug.

@rcarriga Do you have any tips for figuring out where this control.wait is getting called from?

Providing stack traces is actually a big thing I tried to do in nio because I had the same experience of things just failing mysteriously 😅

The reason there's no trace here is because it's not nio that is catching the error, it is this code:
https://github.com/stevearc/overseer.nvim/blob/1ddb3671b7198b64c73824a5e5e21f8eb2ab47c3/lua/overseer/task.lua#L445-L450

Changing the code to use xpcall would allow you to get the traceback and place it in the message

local ok, result = xpcall(comp[name], function (err)
  log:error("Task %s dispatch %s.%s: %s\n%s", self.name, comp.name, name, err, debug.traceback(err, 2))
end, comp, self, ...)
if ok and result ~= nil then
  table.insert(ret, result)
end

The error is happening because you're using queue.put which is an asynchronous function within the callback for on_output which is not called in an asynchronous context.
https://github.com/stevearc/overseer.nvim/blob/1ddb3671b7198b64c73824a5e5e21f8eb2ab47c3/lua/neotest/client/strategies/overseer.lua#L115
Changing to put_nowait should fix the issue 😄

Calling wrapped functions is supported to allow users to be able to use them in non-async contexts with the usual callback style without having to choose between an async version and synchronous version. This has the disadvantage of not giving a very helpful error because it's be up to the wrapped function to check for callback. Nio could check that a callback is provided in non-async contexts and error if it's not but that would prevent uses where a callback is optional. As a middleground we can log a warning to nio's log file.

rcarriga added a commit to rcarriga/neotest-dotnet that referenced this pull request Apr 11, 2023
Just a small change to adapt to the new async library in neotest.

Depends on nvim-neotest/neotest#228
@stevearc
Copy link
Copy Markdown
Contributor

Thanks for the help! The overseer PR is ready to go with full support for the new nio library.

@Issafalcon
Copy link
Copy Markdown

Hi @rcarriga - Thanks for the update.

I've tested your changes on my end. All looks fine to me. The (few) tests I have still pass also.

@rcarriga rcarriga force-pushed the feat/nio-async-library branch from 500732c to 299101b Compare April 15, 2023 09:46
BREAKING CHANGE: Removes usage of plenary's async library which is not
compatible with the new library.
Tasks without a callback that fail will now raise the error to the user
to prevent silent failures.

Consumers APIs now use nio.create to wrap async functionality so users
can choose to wait for results with callbacks or in async contexts.
@rcarriga rcarriga force-pushed the feat/nio-async-library branch from 299101b to da4f02a Compare April 15, 2023 10:12
@rcarriga
Copy link
Copy Markdown
Collaborator Author

Brilliant, since we're looking good I'll get to merging this

@rcarriga rcarriga merged commit 3dbac1e into master Apr 15, 2023
@rcarriga rcarriga deleted the feat/nio-async-library branch April 15, 2023 10:14
rouge8 added a commit to rouge8/neotest-rust that referenced this pull request Apr 15, 2023
zidhuss added a commit to zidhuss/neotest-minitest that referenced this pull request Apr 24, 2023
Tests were broken since we still using `require("plenary.async.tests")`.
This was no longer working after nvim-neotest/neotest#228
MarkEmmons pushed a commit to MarkEmmons/neotest-rust that referenced this pull request Apr 25, 2023
Per the comments in nvim-neotest/neotest#228,
adapters using "plenary.async.tests" will have broken tests. This is
remedied by using "neotest.async", which will import the nio library
once 228 is merged.

I've also switched from using TSInstallSync to TSUpdateSync for
installing the rust parser in the makefile. TSUpdateSync will still
install the parser if it does not exist but will not hang when running
'make test' on a system where it is already installed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants