Small idea: make AsyncHandle::new unsafe or take a badge-like type

[andry-dev]

Hi, I'm working on a plugin for handling async system events in Neovim, see: https://git.sr.ht/~anri/system_events.nvim.

While nvim-oxi is a really cool project and I could not recommend it more for building these sort of plugins, I find the current API allows a particularly annoying footgun: when using libuv, AsyncHandle must be created from the main thread, otherwise Neovim panics.
My mind knows about and totally understands this contract, sure, but when refactoring the plugin above sometimes I accidentally do stupid stuff (like creating an handle in another thread) --- with the current API the compiler happily allows it because it doesn't understand the underlying contract, but then Neovim crashes with no chance to recover.

I've resorted to this sort of hack:

static MAIN_THREAD_ID: OnceLock<ThreadId> = OnceLock::new();

struct Uninitialized;
struct Initialized;
struct Usable;

// Empty type which is !Send and !Sync
// The Usable state signals that the code was called from the main thread
struct MainThreadToken<State = Usable> { /* ... */ }

impl MainThreadToken<Uninitialized> {
  // *Unsafe* function which inits the static above
  // and returns a MainThreadToken<Initialized>
}

impl MainThreadToken<Initialized> {
  // Function which checks that
  // std::thread::current().id() == MAIN_THREAD_ID
  // and, if so, returns Ok(MainThreadToken<Usable>) otherwise Err(...)
}

// In another file that wraps AsyncHandle + UnboundedReceiver

struct NvimChannel<T> { /* ... */ }

impl NvimChannel<T> {
  fn new(_token: MainThreadToken<Usable>, f: impl Fn(...) -> ...) -> Self { /* ... */ }
}

This way, it's impossible to instantiate the Handle without initializing it in the main thread. If the conversion between the Initialized -> Usable states fails, then it's at the very least possible to recover for a crash, alert the user and continue working.

While this is pretty much an opinionated setup, a more library-friendly way to do it would be to expose a marker trait that the user can implement, and provide an optional module/feature/crate that does the above for streamlining it. Or just make the function AsyncHandle::new unsafe.

Obviously, if you don't think there is any need for this idea then you're free to close the issue.

[airblast-dev]

While it might work it results in a really annoying to use API for the general case.
I am not disagreeing with the issue but its just not that simple to solve.

I am working on a similar library to nvim-oxi (see disclaimer). My approach was to set various flags in callbacks and entrypoints and then perform a quick check to validate them in call sites and panic if we are calling something when we shouldn't.

This makes sense until thread B calls a function when thread A has access. To avoid that I used thread locals so a thread by default has no access and it is given access inside wrappers and is revoked access upon leaving the callback (the user doesn't see any of this). Part of the reason for doing this was for optimizations since we are able to do some interesting stuff when we can easily check if we can call the functions without causing a panic. (the panic could be replaced with a result but you get the idea)

Same logic should be able to be extended for async as well where async entrypoints set a thread local marker and then checks if access was provided in this thread. That said I haven't worked with the async side of neovim yet so maybe this wouldn't work for some reason I am not aware of.

full disclaimer: I am working on a library similar to nvim-oxi. The goals and scope of the project are much different and half of the API is not implemented yet so don't bother checking it out.

Edit: while your issue is for AsyncHandle::new the problem is actually present for all functions. Even without a crash its definitely unsound as we (mostly) aren't expected to be calling functions or reading values unless execution is yielded - so things can be in an invalid state or even be mutated whilst we are reading from them.

[andry-dev]

While it might work it results in a really annoying to use API for the general case.
Edit: while your issue is for AsyncHandle::new the problem is actually present for all functions. Even without a crash its definitely unsound as we (mostly) aren't expected to be calling functions or reading values unless execution is yielded - so things can be in an invalid state or even be mutated whilst we are reading from them.

Thank you for your reply, I didn't realize that basically the whole exposed API was unsound; it so happens that I never needed to call it from another thread, and I use tracing for logging from the other thread anyway. Since I was only concerned with AsyncHandle it didn't seem too bad, but I see that applying it to the whole API would indeed be annoying :/

[...] This makes sense until thread B calls a function when thread A has access. To avoid that I used thread locals [...]

I don't know enough about Neovim and Lua internals to understand exactly what you're saying: it appears that you can call the functions from another thread, but you need the main thread to hand hover some state (?) to not invalidate Lua's internal state.

[airblast-dev]

I don't know enough about Neovim and Lua internals to understand exactly what you're saying

Not your fault. There is so many traps in neovim's C API and none of them are documented (they don't officially support the API yet).

it appears that you can call the functions from another thread, but you need the main thread to hand hover some state (?) to not invalidate Lua's internal state.

Essentially never call neovim's functions outside of callbacks and plugin entrypoints and never call neovim's functions from another thread. To my knowledge all exposed functions are safe to call from another thread as long as main execution isn't returned and another thread doesn't call another neovim function, but there is nothing that guarantees it.

There is the Lua side of things which you mentioned and is true. The actual problem is that nearly none of the functions are reentrant or thread safe. Neovim's uses a ton of mutable statics in the code base which means only a single neovim function can be called at a time if we don't want race conditions and many other horrors. Even then we have the possible usage of thread locals in neovim which makes calling functions from other threads even riskier.

I have no idea how things work with libuv so can't really speak on that side of things. There are multiple ways to fix it, just none that I am aware of that provides a sane API and compile time checks. Which leaves us with similar solutions to the one I described above where we have runtime errors or panics. (checking the thread id like you mentioned is also an option but its also very slow)

[noib3]

Yes, this is a known issue that could be better documented.

The implicit API contract is that all free-standing functions exposed by this library should only be called from the main thread. Like @airblast-dev mentioned, even though calling one of the nvim_oxi::api::* functions from another thread may not result in an immediate segfault, it opens up all sorts of potential race conditions and other synchronization issues.

While I was designing the library's API I thought about providing some &Neovim parameter to the plugin's entrypoint, and then having all bindings defined as methods on that struct instead. That is, having:

#[nvim_oxi::plugin]
fn entrypoint(nvim: &Neovim) {
    nvim.api().command("Hi!")
}

instead of:

#[nvim_oxi::plugin]
fn entrypoint() {
    nvim_oxi::api::command("Hi!")
}

Where Neovim is just some !Send and !Sync type like struct Neovim(PhandomData<*mut ()>) which ensures all functions can only be called from the main thread.

In the end I decided not to do this as I thought it would lead to entire codebases being littered with the same nvim parameter having to be passed all over the place. It would also have to be passed to all user-provided callbacks, etc. I believe this is similar to what mlua does with their Lua parameter.

Another reason I didn't do this was that it's easy to build this method-based API on top of the free-standing-based API provided by nvim-oxi, but doing the opposite is not possible (at least without some global state and unsafe bits).