GNOME Bugzilla – Bug 739234
Tracker sometimes ignores SIGINT/SIGTERM
Last modified: 2014-12-27 16:38:58 UTC
Test cases in functional-tests/17-ontology-changes.py sometimes fail with timeout, waiting for tracker-store to exit after killing it with SIGTERM - this is sometimes not processed properly in tracker-store beause it calls g_main_loop_quit() from a regular UNIX signal handler. Similar way to handle signals is used in other tracker processes too. Documentation for g_unix_signal_source_new() explains why it is not safe to call g_main_loop_quit() from a regular UNIX signal handler.
Created attachment 289400 [details] [review] [PATCH] Use g_unix_signal_add() for signal handlers
Comment on attachment 289400 [details] [review] [PATCH] Use g_unix_signal_add() for signal handlers >From c8d7019571a37f8ccb68f508500a9d0e6adffe11 Mon Sep 17 00:00:00 2001 >From: Martin Kampas <martin.kampas@tieto.com> >Date: Mon, 27 Oct 2014 07:37:50 +0100 >Subject: [PATCH] Use g_unix_signal_add() for signal handlers > >[tracker] Do not randomly ignore SIGINT/SIGTERM > >Identified by functional-tests/17-ontology-changes timeouting randomly. > >Documentation for g_unix_signal_source_new() explains why it is not safe >to call g_main_loop_quit() from a regular UNIX signal handler. I read the documentation and thought it was quite bad (personally) that you can't use the UNIX API if using a GLib main loop. >Intentionally removed the (main_loop != NULL) tests - this cannot happen. OK. >diff --git a/src/miners/apps/tracker-main.c b/src/miners/apps/tracker-main.c >index d3f42a9..963525b 100644 >--- a/src/miners/apps/tracker-main.c >+++ b/src/miners/apps/tracker-main.c >@@ -67,8 +67,8 @@ static GOptionEntry entries[] = { > { NULL } > }; > >-static void >-signal_handler (int signo) >+static gboolean >+signal_handler (void) > { > static gboolean in_loop = FALSE; > >@@ -77,42 +77,21 @@ signal_handler (int signo) > _exit (EXIT_FAILURE); > } > >- switch (signo) { >- case SIGTERM: >- case SIGINT: >- in_loop = TRUE; >- if (main_loop != NULL) { >- g_main_loop_quit (main_loop); >- } else { >- exit (0); >- } >- /* Fall through */ >- default: >- if (g_strsignal (signo)) { >- g_print ("\n"); >- g_print ("Received signal:%d->'%s'\n", >- signo, >- g_strsignal (signo)); >- } >- break; >- } >+ in_loop = TRUE; >+ g_main_loop_quit (main_loop); >+ >+ g_print ("\n"); >+ g_print ("Received signal"); My main complaint here is that we don't know *what* signal is received without separate signal handlers and I think it's important that we log/know what a process was signalled before it quit for debugging and understanding bug reports we get. I also think just having "Received signal" is quite uninformative on its own. Perhaps we could have a function that does the leg work and call it from multiple callbacks for different signal types. I am surprised GLib doesn't tell you in the callback what signal you got, but then they're re-using the GSource framework I suppose. > static void > initialize_signal_handler (void) > { > #ifndef G_OS_WIN32 >- struct sigaction act; >- sigset_t empty_mask; >- >- sigemptyset (&empty_mask); >- act.sa_handler = signal_handler; >- act.sa_mask = empty_mask; >- act.sa_flags = 0; >- >- sigaction (SIGTERM, &act, NULL); >- sigaction (SIGINT, &act, NULL); >- sigaction (SIGHUP, &act, NULL); >+ g_unix_signal_add (SIGTERM, signal_handler, NULL); >+ g_unix_signal_add (SIGINT, signal_handler, NULL); > #endif /* G_OS_WIN32 */ I am glad we're fixing this really, this is one of the only WIN32 checks we have and it's pointless. The SIGHUP was going to be used to re-read config, but we don't need that with inotify - nice catch :) >@@ -393,10 +375,11 @@ main (int argc, char *argv[]) > controller = tracker_extract_controller_new (decorator); > tracker_miner_start (TRACKER_MINER (decorator)); > >- initialize_signal_handler (); >- > /* Main loop */ > main_loop = g_main_loop_new (NULL, FALSE); >+ >+ initialize_signal_handler (); >+ > g_main_loop_run (main_loop); > > my_main_loop = main_loop; The rest looks good, thanks for the patch Martin!
Created attachment 289565 [details] [review] [PATCH] Use g_unix_signal_add() for signal handlers Patch updated to pass signum via user_data to signal handler.
Comment on attachment 289565 [details] [review] [PATCH] Use g_unix_signal_add() for signal handlers Thanks for the update Martin, looks good to me!
Comment on attachment 289565 [details] [review] [PATCH] Use g_unix_signal_add() for signal handlers Had to fix some things since the new tracker-control --> tracker binary change. Working now. There were some missing includes too.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.