[RFC] Move signal handling to libuv event loop #410
Conversation
|
This is interesting. I avoid the async-signal-safe madness in my own (linux-only) apps by using signalfd. The functionality provided by libuv seems similar. Perhaps they are using the self-pipe trick as a fallback on other unices? Admittedly I don't know enough about the original vim signal handling to say if we're not missing out on anything important by removing all the crazy core dump, reallyexit and other things... EDIT: just confirmed they're using a variant of the self-pipe trick. The djb is strong in them... |
|
@tarruda This PR is cool: It somehow fixes the failing unit test on my system I reported earlier. |
|
One problem of this PR is that vim has Most of the times that cannot be the case, since when the terminal is in raw mode(most of the times it should be) ctrl+c doesn't generate a SIGINT and vim has to read the key sequence explicitly, but I still need to make sure libuv is polled every time the terminal is set to cooked(that should probably be fixed by adding a calls to |
|
I used the following test: while 1
let c = 123
endwhileThat inifinite loop is always interrupted by sending an INT signal from another terminal, no matter if its passed as a -c argument or if vim is in ex mode so my guess is that vim always polls for characters(and consequently for signals) which should make this PR ok |
|
Awesome! |
|
CTRL-C in insert-mode changes at once into normal mode, but |
|
@oni-link Yes that would be a good way to handle with the situation |
|
Doesn't this cause a change in behavior where neovim will now not try to flush swapfiles, etc. (i.e., everything done in preserve_exit) except when SIGTERM/SIGQUIT/SIGHUP are received? Previously Vim would call preserve_exit for any deadly signal. |
|
@jamessan The question is: Should vim continue running on a fatal error such as a SIGSEGV/SIGABRT? It's very likely those are caused by bugs so perhaps the safest action is the default one: exit as soon as possible and generate a core dump. Considering that vim already flushes swapfiles every 4 seconds, the loss of work, if any, wouldn't be significant. On the other hand, by assuming that we only handle normal signals such as INT/HUP/TERM, we can execute unsafe actions in the deadly handler. The VimLeave autocommand will be much more useful like this. |
|
If no one objects these changes, I will merge them tomorrow |
|
@tarruda I did some resizings, I attached gdb to it. See http://pastie.org/8975268 |
|
@tarruda previous |
|
@philix how to reproduce this freeze? Does the freeze happens in the |
|
I'm resizing very fast with the keyboard but still couldn't reproduce, I wonder if this is OSX-specific. @stefan991 can you reproduce this? @philix when you said the previous revision didn't freeze, you meant the revision on master without this new signal handling code right? |
|
@tarruda Got this while compiling the last commit: using commit 1567cc5 i could reproduce the resizing bug. Neovim dosn't handle input any more, but resiszing does continue to work. It takes some tries, but I could reproduce it more than 5 times. One time I had the case that the input took some seconds to show up. But I could only reproduce it with my |
|
@tarruda: I think I found the stack trace that causes Neovim to ignore input after resizing: (using commit 1567cc5)
|
|
Just out of curiosity, how did you get that stacktrace? Checking out the Actually, I just noticed that your stacktrace calls (notes for me personally)
|
|
@aktau yes, I did add symbolic breakpoints for |
|
@stefan991 Thanks for debugging this for me. At first I didn't consider the possibility of |
|
@stefan991 / @philix does that last commit fixes the freeze bug? Also if anyone has a better way of working around this issue I'm all ears. Personally I hate how vim uses global variables to make functions behave differently on recursion, but I couldn't find a better solution here. The problem is that vim event loop isn't sane in the sense that it needs to call itself |
|
@tarruda: I'm getting |
|
@stefan991 what steps did you take to reproduce this? Only a simple resize? |
|
@stefan991 are you running this with |
| if (entered++) { | ||
| // We don't want recursion because the loop should only start once, for | ||
| // now use this hack to prevent it. | ||
| return false; |
There was a problem hiding this comment.
Is it possible that recursively two calls to poll functions, both with ms < 0, are made? Would the caller not expect a result for the second call?
A call graph for this scenario (no input made):
os_inchar(...,ms=-1,...) ->
inbuf_poll(-1) ->
resize (results in a second blocking *_poll call) ->
os_inchar(...,ms=-1,...) ->
inbuf_poll(-1) ->
read_from_input_buf() ->
fill_input_buf(exit_on_error) ->
read_error_exit()
There was a problem hiding this comment.
True, I guess this problem needs to be handled differently
|
@tarruda: I resized the terminal window several seconds with the mouse. Couldn't reproduce this with |
|
@tarruda: Ack didn't find an occurrence of |
|
@tarruda: looks good on OSX. |
|
@tarruda looks good on OSX. No resize/redraw issues. |
|
I've opened #434 to deal with the data structure boilerplate, lets leave it like this for now and use a separate PR after the library is decided. I've also removed the K_EVENT special key and left all the event handling code into event.c. The main goal of K_EVENT was to avoid recursion of libuv The last update is working well on linux, if it doesn't break anything for OS X users then I will squash/merge. |
|
|
With the following nvimrc set nocompatible
filetype off
set rtp+=~/.vim/bundle/vundle/
call vundle#rc()
Bundle 'gmarik/vundle'
Bundle 'itchyny/lightline.vim'
syntax on
filetype plugin indent on
set laststatus=2
|
The problem was recursion into
Yes need to take that into account
I think the last commit should have fixed it(Can you confirm?) As for the abort, it was caused by |
After the first SIGINT you still need another signal or input if |
|
@oni-link What do you think about this latest version? Did it address every item in your list correctly? |
|
@tarruda The exit condition in the Is it okay to leave the event queue unprocessed if exiting the loop? The stacking timeout problem would be solved with this solution. You can always exit the loop for |
|
@oni-link I understand the I dont see a problem with leaving the queue unprocessed, but I cant see the stacking timeout problem you mention, can you give an example? |
|
@tarruda I try to construct a case where you would exit. 'Stacking timeout problem': That was a problem before 8a2bdec. |
|
@oni-link I missed that case, thanks for showing me. I have refactored the event_poll loop, it should be simpler to understand and it also subtracts the ellapsed time in case of ms > 0 |
|
Squash/rebase |
| { | ||
| Event *event = (Event *)malloc(sizeof(Event)); | ||
| event->type = kEventSignal; | ||
| event->data = malloc(sizeof(int)); |
|
It seem pretty stable, and now we can use I will do some more tests and will squash/merge it the next half-hour or so |
This removes all signal handling code from os_unix.c to os/signal.c. Now signal handling is done like this: - Watchers for signals are registered with libuv default event loop - `event_poll` continuously calls `poll_uv_loop` to produce events until it receives user input, SIGINT or a timeout - Any signals received in `poll_uv_loop` will push events to a queue that is drained and processed by `event_poll` Signals aren't handled directly in the libuv callback to avoid recursion in the event loop(which isn't supported by libuv). The same principle will apply to other events in the future: Push to a queue from a libuv callback and drain it from `event_poll`
|
done, now move on to the shell/co-process infrastructure |
\o/ |
- Add channel module that exposes the API over arbitrary streams - Add `xmemdup` for duplicating memory chunks - Make job exit callback optional
Problem: Vim doesn't recognize all htmldjango files. Solution: Recognize a comment. (Daniel Hahler, PR #410) vim/vim@d8986fd
Problem: Vim doesn't recognize all htmldjango files. Solution: Recognize a comment. (Daniel Hahler, PR neovim#410) vim/vim@d8986fd
This removes all signal handling code from os_unix.c to os/signal.c, which
registers watchers on libuv default loop. Now signal handling will only happen
when Neovim transfers control to libuv, and SIG{HUP,TERM,QUIT} are the only
deadly signals intercepted.
I removed excessive signal handling code. For example, before it tried to handle SIGABRT which normally generates a core dump by exiting cleanly. Vim only generated core core dumps explicitly with
may_core_dumpfunction, and if the signal was received multiple times. This shouldn't happen with libuv since signals are delivered synchronously tosignal_cband only when libuv is polling for events. Signals such as SIGABRT normally mean program errors(eg: failed assertions) and should probably exit imediatelly. SIG{HUP,TERM,QUIT} are the only deadly signals we are handling because these are not related to bugs.There may still be a resize problem with the
RealWaitForCharinmch_call_shellsince that still usesselect/polldirectly, but it should be fixed whenmch_call_shellis reimplemented with libuv process facilities@philix / @stefan991 Are the OS X redrawing problems fixed in this branch?