Skip to content
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

SIGINT forwarding for RPC / ctrl-c interrupt rpcrequest() #7546

Open
SevereOverfl0w opened this issue Nov 12, 2017 · 12 comments
Open

SIGINT forwarding for RPC / ctrl-c interrupt rpcrequest() #7546

SevereOverfl0w opened this issue Nov 12, 2017 · 12 comments
Labels
enhancement feature request remote-plugin plugins as RPC coprocesses (node.js, python, etc) rpc ux user experience
Milestone

Comments

@SevereOverfl0w
Copy link

It would be useful to have either:

  1. SIGINT forwarding
  2. Subscribe to SIGINT on RPC

Each have trade-offs, perhaps being strongly down to a language-by-language interface.

This would allow for the interruption of long-running commands. Much like the old-style python interface can do.

@bfredl
Copy link
Member

bfredl commented Nov 12, 2017

I think SIGINT forwarding is better. It is more powerful (can interrupt more things) and the second option can be more or less emulated with signalfd.

@SevereOverfl0w
Copy link
Author

There's also a suggestion, by @bfredl, that double tapping Ctrl-C could kill the underlying process and close the channel.

@bfredl
Copy link
Member

bfredl commented Nov 12, 2017

closing the channel might be enough, or at least give the process some time to clean up itself before actually killing it.

@SevereOverfl0w
Copy link
Author

Perhaps the process could be sent something like SIGTERM for the second Ctrl-C? Which, I believe, can be hooked to perform cleanup. man kill seems to suggest this is the case.

@bfredl
Copy link
Member

bfredl commented Nov 12, 2017

Aborting the request solved the immediate problem: nvim is responsive to the user again. Now the RPC state is out sync and therefore the simplest way is to close the RPC channel. But there is no need to kill the process right away. We shouldn't be more aggressive than necessary.

@justinmk justinmk added enhancement feature request rpc remote-plugin plugins as RPC coprocesses (node.js, python, etc) ux user experience labels Nov 13, 2017
@justinmk justinmk added this to the 0.3 milestone Nov 13, 2017
@justinmk justinmk changed the title SIGINT forwarding for RPC SIGINT forwarding for RPC / ctrl-c interrupt rpcrequest() Jul 2, 2019
@ddickstein
Copy link
Contributor

ddickstein commented May 5, 2021

I'm not sure forwarding SIGINT to the plugin process expresses the right intention. When the user presses Ctrl-C in Neovim, they mean to say "stop what you're doing and return control to me." This isn't the same as an intention to kill the plugin process. Sending a SIGINT in some sense requires knowledge about the plugin process -- knowledge that either it's fine to bring down when the user presses Ctrl-C, or knowledge that the process handles SIGINT. I don't think either of these is a safe assumption for Neovim to make. Non-trivial processes likely both (A) want to support the user aborting an RPC request without going down, and (B) want to go down upon receiving SIGINT.

I think what should probably happen is a keyboard interrupt notification is asynchronously sent over the channel. The plugin can listen and decide how to respond. This already happens if during an rpcrequest the plugin sends a nested async notification after the user has pressed Ctrl-C - it will receive a notification back communicating that keyboard interrupt. But no proactive communication of the Ctrl-C to the plugin happens right now until the plugin next tries to talk to Neovim. In VCaml we are working around this by heartbeating async notifications.

If despite these reservations the plans for SIGINT move forward, can clients set a flag to opt out? I would be very sad if to write a Neovim plugin I needed to write a signal handler.

@bfredl
Copy link
Member

bfredl commented May 5, 2021

@ddickstein sure, closing the channel should only happen on the second SIGINT, giving the process a chance to handle it cleanly and only interrupt the request (the host library returning an error unless the specific plugin handles it).

I think what should probably happen is a keyboard interrupt notification is asynchronously sent on the event bus. The plugin can listen and decide how to respond. [...] would be very sad if to write a Neovim plugin I needed to write a signal handler.

The best mechanism might depend on the language. The problem with using socket IO is that if the process is blocked in a CPU loop not doing IO, it won't get the message from neovim.. Many runtime environments like cpython already converts the signal safely into into an ordinary exception of the language, which could be handled adequately by plugin code. Is the situation reverse in ocaml (no safe interrupt exceptions, but preemptive IO handling)?

@ddickstein
Copy link
Contributor

ddickstein commented May 5, 2021

Ah you make an interesting point! So VCaml uses Async, and the logic for returning control to the user waits for either a keyboard interrupt or for the request to finish processing. So as long as the Async scheduler isn't being starved, it will be able to run the logic that detects the keyboard interrupt and returns control. But if there is a job w/ a long synchronous block of code, the other jobs in the queue will be stuck waiting until that job returns control to the scheduler. A blocking call from rpcrequest doesn't imply synchronous logic that blocks Async, but it's still something that could happen depending on how the code is written. Jobs that take a long time for other reasons, e.g., shelling out to some other program to run some command that takes a while, will not block the scheduler and therefore will not cause an issue.

I am by no means an expert on signal handlers, but it looks like these can be optionally handled by Async. If they are, I believe the signal handler runs as an ordinary Async job, which is nicer to reason about but has the same limitation re: starved scheduler. If they aren't, I think the signal handler is invoked whenever the signal is received in a separate thread, which has that preemptive behavior. The signal can be converted into a safe exception - here's an example (this doesn't use the Core library, but same idea):

exception Break;;
let break _ = raise Break;;
...
let main_loop () =
  signal sigint (Signal_handle break);
  while true do
    try (* Read and evaluate user commands  *)
    with Break -> (* Display "stopped" *)
  done;;

Also from that page, on OCaml's native signal handling:

In fact, OCaml does not treat signals in a strictly asynchronous fashion. On receiving a signal, OCaml records the receipt of the signal but the signal handling function will only be executed at certain checkpoints. These are frequent enough to provide the illusion of asynchronous execution. The checkpoints typically occur during allocations, loop controls, or interactions with the system (particularly system calls). OCaml guarantees that a program that does not loop, does not allocate, and does not interact with the system will not have its execution interleaved with that of a signal handler. In particular, storing an unallocated value (integer, boolean, etc. — but not a float!) in a reference cell cannot result in the race condition described above.

All of this said, I'm not sure I understand why the notification itself needs to be preemptive. It seems to me that Neovim should itself forcibly return from rpcrequest when the user presses Ctrl-C (maybe as though the RPC had returned a Nil response), but then just send a regular notification over RPC to announce that this has happened. I don't think Neovim should need to plead with plugins to release their grip.

@bfredl
Copy link
Member

bfredl commented May 5, 2021

It seems to me that Neovim should itself forcibly return from rpcrequest when the user presses Ctrl-C (maybe as though the RPC had returned a Nil response), but then just send a regular notification over RPC to announce that this has happened. I don't think Neovim should need to plead with plugins to release their grip.

Because, within the limitations of the present msgpack-rpc protocol, it is tricky to do that while keeping RPC in a synchronized state, so for most clients this would entail closing the channel. The idea here is to give the host a chance to handle the interrupt gracefully without needing the active cooperation of all plugins, and avoid forcibly closing the channel as long as the host cooperates. In case of a process getting stuck the second CTRL-C hit within the same rpcrequest will unconditionally abort the request.

@ddickstein
Copy link
Contributor

ddickstein commented May 6, 2021

within the limitations of the present msgpack-rpc protocol, it is tricky to do that while keeping RPC in a synchronized state, so for most clients this would entail closing the channel.

Can you elaborate on these limitations? It seems to me that message type 2 (async notification) can be sent at any time from either party, so I'm not sure why sending one would cause the RPC to fall out of a synchronized state. And you'd only need to send the Ctrl-C to the particular client on which rpcrequest is blocking, so I don't think cross-plugin cooperation is needed.

@bfredl
Copy link
Member

bfredl commented May 6, 2021

It seems to me that message type 2 (async notification) can be sent at any time from either party, so I'm not sure why sending one would cause the RPC to fall out of a synchronized state.

I do not think I have claimed this either.

And you'd only need to send the Ctrl-C to the particular client on which rpcrequest is blocking, so I don't think cross-plugin cooperation is needed.

The Idea is that the host (or at least client library, in case of single-process plugins) provides reasonable defaults so reasonably written plugin code will do the right thing most of the time even when not actively keeping interrupts in mind all of the time. for instance in python a KeyboardInterupt exception is supposed to bubble up from the plugin code to the host which can handle it properly (unless the infamous unqualified except: footgun was used).

@ddickstein
Copy link
Contributor

Ok. I think I accept the argument for preemptively alerting the plugin process, particularly for plugins that are written without an event loop that frequently polls its file descriptors, esp. so they don't issue further RPC requests to Neovim as part of processing the same request.

I was talking about these designs with @dalyadickstein and she pointed out that there might also be surprises in a design with different key presses meaning different things - someone who presses ctrl-c once may not know pressing it a second time will help, and someone who presses it multiple times (e.g., in frustration or panic) may not expect the second press to have different semantics (more aggressive than just aborting the request in flight). Documentation mitigates this only somewhat.

She suggested a hybrid of the designs we've discussed here:

  • Send a SIGINT to the process on which we are waiting.
  • Control is always released to the user. If the process does not reply in a given time frame (probably something on the order of a hundred milliseconds - long enough for the process to send a response but still instantaneous for the human), fake a Nil response from the process to unblock and drop the process' response when it arrives (to keep the RPC state in sync).

@clason clason modified the milestones: 0.6, 0.7 Nov 30, 2021
@bfredl bfredl modified the milestones: 0.9, 0.10 Apr 7, 2023
@dundargoc dundargoc modified the milestones: 0.10, backlog Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature request remote-plugin plugins as RPC coprocesses (node.js, python, etc) rpc ux user experience
Projects
None yet
Development

No branches or pull requests

6 participants