After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 704322 - glib-unix: fix handling of multiple signal source for the same signal
glib-unix: fix handling of multiple signal source for the same signal
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-07-16 13:31 UTC by Giovanni Campagna
Modified: 2013-10-29 17:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glib-unix: fix handling of multiple signal source for the same signal (5.08 KB, patch)
2013-07-16 13:31 UTC, Giovanni Campagna
committed Details | Review

Description Giovanni Campagna 2013-07-16 13:31:50 UTC
We can't reset the pending flag for a signal until we've traversed
the whole list, as the documentation clearly says that in case multiple
sources they all get invoked.
This is still racy if you get a signal after checking the flag
but before resetting it, but it was the same before. The correct
fix would be to use sigwait() or signalfd(), but that would mean
blocking all signals in all threads, which is not compatible
with existing applications.
Comment 1 Giovanni Campagna 2013-07-16 13:31:52 UTC
Created attachment 249272 [details] [review]
glib-unix: fix handling of multiple signal source for the same signal
Comment 2 Allison Karlitskaya (desrt) 2013-07-16 13:48:44 UTC
Where do the docs say that an incoming unix signal is delivered to all of the sources for that signal?
Comment 3 Giovanni Campagna 2013-07-16 14:20:26 UTC
Note that unlike the UNIX default, all sources which have created a watch will be dispatched, regardless of which underlying thread invoked g_unix_signal_source_new().

From the docs for g_unix_signal_source_new()
Comment 4 Allison Karlitskaya (desrt) 2013-07-16 14:58:01 UTC
To me this speaks to the question of which thread signals will be processed in rather than having multiple instances of handlers.  Clearly it never worked that way before, so the code doesn't match your reading of the docs.

Why do you want multiple handlers?
Comment 5 Colin Walters 2013-07-18 21:57:04 UTC
(In reply to comment #4)
> To me this speaks to the question of which thread signals will be processed in
> rather than having multiple instances of handlers.  Clearly it never worked
> that way before, so the code doesn't match your reading of the docs.

That was my original intention...looks like I got it wrong.

> Why do you want multiple handlers?

It wouldn't seem totally crazy to me for a library (sufficiently documented) to register a SIGTERM handler for cleanup.  I dunno, there's also the situation where your application just grows over time and you end up with two SIGTERM/SIGINT handlers just in your app.

Having to refactor to hoist things to the toplevel can be a pain.
Comment 6 Colin Walters 2013-07-18 23:25:14 UTC
Review of attachment 249272 [details] [review]:

This code looks good.

::: glib/tests/unix.c
@@ +137,3 @@
+
+  kill (getpid (), signum);
+  g_main_loop_run (mainloop);

The new idiomatic mainloop code here would be:

while (sig_counter != 2)
  g_main_context_iteration (NULL);

Up to you if you want to convert though.
Comment 7 Giovanni Campagna 2013-07-19 07:35:34 UTC
Attachment 249272 [details] pushed as be2c7b8 - glib-unix: fix handling of multiple signal source for the same signal

I don't want to rewrite the existing test cases, and for consistency
I left the new one as is.
Comment 8 Allison Karlitskaya (desrt) 2013-10-29 17:52:26 UTC
This caused bug 711090.