Now that a new enough GLib is available the signal can be handled cleanly on the main loop using the GSource for Unix signals. This replaces the illegal SIGTERM handling that was disabled in fbb89f523af47b35e238678d348cfa98e56c760a.
In the future, we might also like to handle `SIGPIPE` which was ignored in cc511a78d80fde742e1794c32b2738ae8ddd4792 to log the error instead of just ignoring it (or handling `write()` failures at their source). You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1255
-- Commit Summary --
* Re-enable SIGTERM handling
-- File Changes --
M src/libmain.c (21)
-- Patch Links --
https://github.com/geany/geany/pull/1255.patch https://github.com/geany/geany/pull/1255.diff
codebrainz commented on this pull request.
main_quit();
} + return G_SOURCE_REMOVE;
I wasn't sure if this should be `G_SOURCE_REMOVE` or `G_SOURCE_CONTINUE`, I guess it doesn't matter since `SIGTERM` is the only signal handled here at this point and the application will exit anyway, but in the future we might like to change the return value depending what signal was handled and what action was taken.
b4n commented on this pull request.
{
+ gint sig = GPOINTER_TO_INT(user_data); if (sig == SIGTERM)
what about having a signal-specific callback instead of checking this? `g_unix_signal_add()` can only get one signum at a time IIUC, so there doesn't seem to be any reason to reuse the same handler when the body will be totally different each time.
So I'd suggest renaming to something like `sigterm_handler()` or something, and remove the signum check (and passing it altogether).
b4n commented on this pull request.
main_quit();
} + return G_SOURCE_REMOVE;
I guess it depends on whether or not returning `CONTINUE` will remove the signal handler altogether and restore the default action or not.
Because IMO * it makes more sense to return `CONTINUE`, unless we *mean* not to handle the signal anymore * it is often customary to stop handling these kind of signals after receiving them for the first time, so that if the app still doesn't quit sending it again will just terminate it, just not necessarily cleanly. So that might suggest `REMOVE`.
Good idea
codebrainz commented on this pull request.
{
+ gint sig = GPOINTER_TO_INT(user_data); if (sig == SIGTERM)
The idea was to make it like a normal signal handler, and since any future handlers are likely going to do something similar, or at least something small/simple, it seemed good to keep all the handling in one place.
At this point it would be cleaner not to pass the signal and just have it `sigterm_handler()` but I intend to submit a pull request "soon" to handle `SIGINT` so the config gets saved when using <kbd>Ctrl</kbd>+<kbd>C</kbd> from the terminal, in which case it's a mere matter of adding `|| sig == SIGINT` in the handler and another call to `g_unix_signal_add()` for `SIGINT`.
If you want, I can change it for now though, and then re-evaluate the code when there's more than one signal.
codebrainz commented on this pull request.
main_quit();
} + return G_SOURCE_REMOVE;
My thinking with `G_SOURCE_REMOVE` was that since it's past the _real_ signal handler, maybe another signal would come and we don't want to handle it twice. I'm not really sure what the right one is.
elextr commented on this pull request.
main_quit();
} + return G_SOURCE_REMOVE;
If Geany ever returns to the main loop during the quit (which at least GIO file saves would do I think) then you should remove it to avoid getting it again. But if you are going to allow multiple signals in the one handler then you may want to continue and then you need code to filter that signal out since you don't want to re-enter `main_quit()` or *bad things* will happen. There is already a quitting global you could use for that :)
Like the sigterm, and sigint to be handling.
Not sure about the sigpipe proposal, I suspect you might get that every time the compiler finishes a build, and that would be annoying to log that each time. Needs to be checked.
Closed #1255 via a108f9162cc98e501e2458cd8b35c499b7f87384.
Feel free to add tweaks to this.
github-comments@lists.geany.org