-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add support for rpcrequest from vim script #4
Add support for rpcrequest from vim script #4
Conversation
This patch lays the foundation for the next PR: using |
7febb91
to
2196e7d
Compare
Hello, thanks for patch. Looks good. I think maybe better to use Option here for functions? Idea is: if i don't need one of this two callbacks, i don't need to write any addition code like empty callback. |
Sounds good. I'll make it happen. |
2196e7d
to
0c307dd
Compare
I'm trying to create a plugin where vimscript sends requests to the rust plugin, and the rust plugin responds to requests. I want vimscript to deliver a "quit" command over RPC to kill the rust plugin (among other commands). "quit" is arbitrary and demonstrates a situation where state is shared between the callback and main thread. To facilitate the "quit" command, I have a The problem I'm having is organizing this [complex?] handler in a closure in such a way that my code remains legible, and I'm not bashing myself against "Expected Type<X>, found Type<closure>" compiler errors in Rust when I refactor my plugin. It kind of sucks that closures require I define them at the time they're passed as arguments. E.g. session.start_event_loop_cb(
Some(|name, args| {
// lots of code here
}),
Some(|name, args| {
// lots of code here
Ok(Value::String("ok".to_owned()))
})); A Trait for the callbacks might alleviate my issue, but that adds more Type complexity due to sharing across threads that affects everyone. I imagine some users of pub trait NeovimHandler {
fn handle_notify (&mut self, name: &str, args: Vec<Value>) {}
fn handle_request (&mut self, name: &str, args: Vec<Values>) -> Result<Value, Value> {
Err(Value::String("Not implemented".to_owned()))
}
} The only other option I know of are trait objects Thoughts? |
I created a version of this patch with a I think this might work out well with a bit more... work. To mitigate complications with shared state, custom For example: // imagine a complex object with lots fields to share
struct SharedState {
// insert much shared state here
}
struct MyHandler {
shared: Arc<Mutex<SharedState>>
}
impl Handler for MyHandler {
fn handle_notify (&mut self, name: &str, args: Vec<Value>) {
let shared = self.shared.lock().unwrap();
// do stuff with `shared`
}
} Anyone not working with shared state isn't forced to mess with I'd also like to keep support for closures by added a ClosureHandler type that takes the closures from Sorry for writing so much. I've spent quite some time with this, and it's hard for me to tackle various solutions without losing track of the problems I've had. |
I think about it a bit and Optional is bad solution for generics as None does not provide type information. So solution with trait maybe ok. |
Ok i think trait with default implementations of handler's ok for given situation. To provide support for closures maybe better to use builder pattern for ClosureHandler without generics pointer, but using dynamic dispatch? ClosureHandler::new().notify_handler(|| {
// something here
}); Or maybe ClosureHandler just not needed :) as implementation of trait is not a big problem, but gives static dispatch as a result. |
If you're happy dropping the closure callbacks, I'm happy. Since I also don't mind keeping support for closures, if desired. It's an interesting exercise to keep the API backward compatible. I'm not sure if the builder pattern gains anything, though maybe it'll drop the I'll update the PR to use the |
0c307dd
to
5c70ee9
Compare
src/rpc/client.rs
Outdated
pub fn start_event_loop_cb<F: FnMut(&str, Vec<Value>) + Send + 'static>(&mut self, cb: F) { | ||
self.dispatch_guard = | ||
Some(Self::dispatch_thread(self.queue.clone(), self.reader.take().unwrap(), cb)); | ||
pub fn start_event_loop_handler<H>(&mut self, handler: Option<H>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daa84 did you want to drop the Option<H>
here and use H
instead? In order to do so, DefaultHandler
must be exposed to the user, so they can pass along that value when they have no need of callbacks.
My first idea with Option was bad, because Option::None for generics does not work good. So i prefer to use two functions with and without handler as was before but with trait as a handler. |
5c70ee9
to
90dd0cc
Compare
Ok, I think I got it this time. |
One last change, can you move out wr.flush() from write_value to encode function? I have plans to move back to rmp value again and think to remove this code :) this is because previously i think Value is removed from rmp value permanently but they are moved this code to separate crate |
Certainly. I'll knock that out sometime Monday. |
Neovim will wait indefinitely for an RPC response that is stuck in the IO buffer of a separate process. Signed-off-by: Justin Charette <charetjc@gmail.com>
Access to the write socket is required for the dispatch thread to send a RPC response back to the requesting vim script. Signed-off-by: Justin Charette <charetjc@gmail.com>
Signed-off-by: Justin Charette <charetjc@gmail.com>
The Handler trait enables users to define handler functions elsewhere, rather than at the time `session.start_event_loop_cb()` is invoked. Moving closures elsewhere causes type mismatch errors in the Rust compiler when `session.start_event_loop_cb()` is invoked. Signed-off-by: Justin Charette <charetjc@gmail.com>
Signed-off-by: Justin Charette <charetjc@gmail.com>
90dd0cc
to
6d913af
Compare
I moved |
One more thing to consider. A user has to define their handler before they can start the session's event loop, and only then can they create the If a handler needs to ask Neovim for more information, how might that be done? The As it stands, it seems the One solution might be to use channel so the handler can communicate with the Rust scope holding onto the Not sure what to do here, or if we can worry about this later, but I just ran into this issue with my test plugin. I can put this in the issue tracker if you prefer. |
It just occurs to me that a single dispatch thread won't be able to process responses from Neovim while that same thread is executing a |
Yes, your right, the idea is: special thread waits for response from neovim and if this thread blocks - response can't be taken. So to process make neovim call it is need to make response to be processed. Currently only sync call implemented, so it's not possible to make call and then go to next instruction. Solution is: take event in event process thread -> send this event (also with id) to channel (don't wait here for response, so thread can continue execution) -> do some process in main thread -> send response with given id in main thread. This way it will work i think :) |
I don't think previously of such use case :( |
No worries. 😄 I tried spawning threads to handle events in If I figure something out that patches Cheers |
Sending events over channels is working very well. Thanks for the suggestion. |
Thanks |
Make the write socket available to the dispatch thread so an rpc
response may be sent back to the requesting vim script.
Rename
cb
tonotify_handler
.Add
request_handler
tostart_event_loop_cb
. Similar tonotify_handler
, but returns a result to be sent in the rpc response.Flush buffered write sockets, otherwise vim will wait indefinitely for
the rpc response.
Not certain whether this is a direction you wanted to go. If not, let me know what you had in mind and I'll try to rewrite this PR to accommodate.