It would make sense to remove the handler and create a new autocommand instead. Autocomands do not give any error messages when absent, require less code for simple thing like source $MYVIMRC (e.g. autocmd SigUSR1 * :source $MYVIMRC) and allow sharing of one event between multiple plugins if needed, though for the last thing they need some agreement regarding how to determine the cause of the signal.

I would also suggest to be consistent and provide both USR1 and USR2 handlers. Also what “known conflicts” you are referring to?

Note regarding names: I can suggest one of three:

autocmd Signal USR1 :command
autocmd SigUSR 1 :command
autocmd SigUSR1 * :command

Personally in favour of the last one: it would be easier to determine whether handler needs to be installed if it would later be chosen to support other events (e.g. autocmd SigTerm, you would not want to default to “just ignore the signal” handler here). Second variant looks the worst, first is not good for trapping deadly signals.

Also you must not call anything from a signal handler, signal may arrive at any time and disturb other activities, this especially applies for non-deadly signals and this especially applies to your code because you are going to run something highly complicated like processing VimL which has lots of interactions with globals*. You need to act like SIGINT/SIGWINCH/SIGALARM: just set a flag and return. Process flag later in the event loop and wherever else you feel necessary.

And you are missing tests.

* : E.g. what if your signal arrives while Vim is in the middle of allocating a list, after l->lv_used_next was assigned, but before first_list was assigned and sourcing vimrc has allocated another lists and stored it in s:list? You will have a situation where some lists would never be garbage collected, but what is worse is that first_list->lv_used_prev->lv_used_next would not be equal to first_list and I could not say that you will not catch a crash due to this.

Wow, thank you all for the great feedback! Using events to trigger autocmds sounds perfect for this. My plan is to add a got_usr1 status variable, register a new SigUSR1 event, and trigger autocmds in main_loop().

@ZyX-I:
The SIGUSR2 conflict I was describing refers to sysmouse using that signal, as noted here. It is leveraged when FEAT_SYSMOUSE is defined, but is otherwise deadly.

@ R0b0t1:
Could you elaborate a bit more on supporting arbitrary signals? With autocmds, each signal would have to have it's own status variable, event and signal handler. Otherwise, it would not be possible to have separate handlers for different signals.

Could you elaborate a bit more on supporting arbitrary signals? With autocmds, each signal would have to have it's own status variable, event and signal handler. Otherwise, it would not be possible to have separate handlers for different signals.

I know that I'm answering by someone else, sorry. But one interesting way to do that would be to use the siginfo structure (save it to reference later to check which signal arrived). If you use sigaction to register a signal you can set the SA_SIGINFO and your handler will get an additional argument (struct signinfo) containing a lot of information about the signal, including the signal number. For it to work I assume that:

plan is to add a got_usr1

This is the plan, but the variable could be renamed to got_monitored_signal and it will catch a number of possible singals (SIGUSR2, SIGINT, SIGTSTP, SIGTTIN, SIGTTOU, SIGPIPE are some that Vim uses and cannot be used at all). Then the main loop would check the saved siginfo structure to see which signal it got.

The bad side is that if two different signals arrive at almost the same time one of them is lost, i.e. only one signal is caught whilst others are lost. Which should not be something bad, since, by design, if two (or more) of the same signal arrive at the same time (or during a period when they're blocked) only one is delivered.

@grochmal got_monitored_signal does not look good. Better have an array of integers, one per signal. It is OK to loose two same signals, but not two different. Also SIGINT may be used: in this case handler needs to set two different flags (got_int does not necessary mean SIGINT), signal handler will be run from main loop after whatever was running was interrupted. (And SIGUSR2 may be used as well because FEAT_SYSMOUSE is not always defined, but I’d rather suggest to not do this.)

Codecov Report

Merging #1718 into master will decrease coverage by 72.41%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master    #1718       +/-   ##
===========================================
- Coverage   74.94%    2.52%   -72.42%     
===========================================
  Files          76       70        -6     
  Lines      124906   117838     -7068     
===========================================
- Hits        93609     2977    -90632     
- Misses      31297   114861    +83564

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a0b06d...eb6e89e. Read the comment docs.

I've updated it to use an autocmd for SigUSR1, though I'm not sure how to proceed with the test. I believe the signal is sent, however because it is not processed until after the function is run, the test fails. Any thoughts on that or a better way to test?

Another downfall with the current implementation is a command has to be entered before the autocmd(s) will be called. In my case, I'm changing the theme and would like it to change without having to go to each session to do something. I was hoping to add extra apply_autocmd calls within do_exmode and normal_cmd, but I'm not sure if there's an appropriate loop I can do so in. I have a feeling solving that would be a step closer to solving the tests as they are, too.

@ZyX-I I like the array idea (as opposed to a shared simple value). I'm thinking an array of structs that contain the signal, event, and received. Then, in the signal handler (which will now be for multiple signals), I find the signal (similar to in the deathtrap) and set received = 1. If I understand correctly, that should be reentrant safe, as only a single write occurs and the array elements are statically placed (need to fact check this though). Then, in main_loop(), do a similar iteration over the array, calling apply_autocmd with the stored event if received == 1. Does that sound about correct?

it's probably better to use a bitset here: it's more flexible (and efficient for all it matters) and it'd make supporting cases like au Signal USR1,INT :command or au Signal * :command really easy.

@LemonBoy Not bitset, never for signals. Unless there is an atomic command “set bit X to 1 at location 0x…” this is going to be “load register, 0x…; or register, mask; store 0x…, register;” and it is completely possible to receive signal after loading, but before storing which is not a problem for integer arrays (there just is one store). The assembler of volatile int bitset = 0; int main(int argc, char **argv) { bitset |= 0x4; return bitset; } compiled with -O3 both with clang and gcc shows that at least my processor does not have “modify bit X at address 0x…” command, or it is not known by clang/gcc.

And I do not think that au Signal USR1,INT is a good idea, not to mention au Signal *. You sometimes need * for debugging, but not for normal operation of plugins. And there is SigUSR1,SigINT for the first case. And I fail to see how it is going to be easier with bitset, from my experience bitsets are always harder to work with, far less flexible and specifically in this case bitsets are going to be completely irrelevant to the implementation. Maybe by “bitset” you mean something else then “integer variable which has different meaning assigned to different bits”? Do not forget that Vim is ANSI C, no std::bitset.

Dealing with the signal only in main_loop() means nothing will happen when in Insert mode.
How about moving this to parse_queued_messages(), which is where various other async calls are handled? The test should then also work using getchar()

Codecov Report

Merging #1718 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1718      +/-   ##
==========================================
+ Coverage   81.83%   81.83%   +<.01%     
==========================================
  Files         133      133              
  Lines      146551   146559       +8     
==========================================
+ Hits       119933   119940       +7     
- Misses      26618    26619       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1824f45...d13b103. Read the comment docs.

@brammool thanks for the parse_queued_messages tip. I rebased this work on top of master and moved the autocmd calling logic there and fixed the test.

Working in insert mode is definitely an improvement. One keypress is required for the autocmd to be called. For a color change, a second keypress is required to update the color, but if I force a redraw in the autocmd, I can avoid the second keypress. Here's the .vimrc I was testing with:

echo("sourcing vimrc")
au SigUSR1 * source $MYVIMRC | redraw
highlight Normal ctermfg=grey ctermbg=black