GNOME Bugzilla – Bug 396958
Glib::signal_idle().connect() not thread-safe
Last modified: 2018-05-22 12:06:23 UTC
Connecting to Glib::signal_idle() or one of the other special signal sources in glibmm (timeout, io, etc) is not thread-safe, as memory visibility rules aren't observed. Details as discussed on the gtkmm mailing list: Now to the bad news. Glib::signal_idle().connect() returns a sigc::connection object. connection::connection(slot_base& sl) : slot_(&sl) { slot_->add_destroy_notify_callback(this, ¬ify); } connection::~connection() { if (slot_) slot_->remove_destroy_notify_callback(this); } Doomed, I'd say. At least the call to remove_destroy_notify_callback() after the GSource mutex has been unlocked is unavoidable with the current API. Maybe we'll be able to play some tricks and put something entirely different into the sigc::connection object. A special-purpose slot which holds a connection ID or something, and goes through the GMainLoop/GSource interface to destroy the callback slot.
I see memory corruption in a real application due to this issue, with valgrind catching invalid writes at the conn_node->install() line. I can't share the application source, unfortunately, but I can test any relevant glibmm patches. Moving the install(source) call to a spot before g_source_attach() does appear to eliminate at least that source of memory corruption. Some thread-related corruption issue still remains, but it is much harder to trigger, and I haven't been able to reproduce it under valgrind.
Patches are welcome. Is this only a problem when using the returned connection object?
(In reply to comment #2) > Is this only a problem when using the returned connection object? Unfortunately, no, since at least the destructor is always invoked.
I'd still like a patch for this. Even if it breaks API it would clarify this for me.
On further thought, I think there may be a different difficulty with this proposal, which is that sigc::trackable does not appear to be thread safe. This problem arises where the slot to be executed represents a non-static member function of a class object derived from sigc::trackable, and it arises because the proposal would involve creating slot object(s) in one or more worker threads, as well as executing the slots in a further different thread (the Glib::MainContext thread) which eventually disposes of the slot, and each act of creation and disposal appears to invoke sigc::trackable functions. The relevant Glib::MainContext thread is likely to be the thread which determines the lifetime of that object, but there is a further vulnerability where that is not the case. I posted one solution to the original problem in this message: http://mail.gnome.org/archives/gtkmm-list/2007-September/msg00009.html but that seems to me to suffer from the same difficulty I have described above. Glib::Dispatcher does not have these issues. (It has slightly different issues of a much lower importance.) Chris
See also bug #512348 Chris
Update: A method connect_once() was been proposed to solve this problem and is now part of glibmm. Unfortunately, the implementation of connect_once() does not appear to take the memory visibility problem into account at all. That is, right now it doesn't solve the problem which led to it being proposed in the first place.
g_idle_add() itself is thread safe. According to http://mail.gnome.org/archives/gtkmm-list/2007-January/msg00145.html , leaving aside the sigc::trackable issue, all that is now needed is to move the call to Glib::SourceConnectionNode::install() to before the call to g_source_attach(). There is still the same sigc::trackable point as arises in bug #512348, which may or may not be a problem, depending on whether you view it only as a documentation issue.
On reflection I think my comment #8 is wrong. There still seems to be an unused sigc::connection object in existence. I think the call needs to be reimplemented rather than rely on sigc::bind_return() and handing it off.
Yes, that's indeed the problem I referred to. Sorry, I should have been more explicit.
Is there a patch in sight?
Chris Vine mentions this indirectly in bug #512348 too.
I've pushed a patch that moves the call to conn_node->install(source) as requested in comment 1 and comment 8. That makes SignalTimeout::connect(), SignalTimeout::connect_seconds(), SignalIdle::connect() and SignalChildWatch:: connect() less thread-unsafe, but not completely thread-safe. The patch also adds a description of the rules to follow because of the thread-unsafety of these methods and of SignalIO::connect(). http://git.gnome.org/browse/glibmm/commit/?id=3aa8e5d2929dceec4f1c0ded5fcbf208c93e1dc9 As mentioned in comment 0, the returned sigc::connection interacts with the sigc::slot in such a way that these connect() methods can't be made thread-safe without thorough modification of libsigc++. This bug has not been completely fixed. I keep it open. If someone wants to close it with resolution WONTFIX, I would not mind.
The sigc::connection returned from the connect() and connect_seconds() methods is not always needed. Similar methods that return void can be made to suffer only from the milder form of thread-unsafety that the connect_once() methods do. See bug 396963 or e.g. the description of Glib::SignalIdle::connect_once() at https://developer.gnome.org/glibmm/stable/classGlib_1_1SignalIdle.html. It's not possible to overload methods that differ only in the return type. New methods returning void can't be named connect(). If someone really needs such methods and can come up with a catchy name, I can make the methods. I want to do this only if someone finds such new methods really useful. I fear that the average user of glibmm can be more confused than helped by a plethora of connect methods (connect, connect_seconds, connect_once, connect_seconds_once, connect_?, connect_seconds_?).
On a general note, I still think it's worth providing thread-safe versions of these methods, as light-weight alternatives to Glib::Dispatcher. E.g. I find myself using Glib::Dispatcher just to notify the main thread that a working thread has finished, which seems like overkill and unnecessarily verbose. Actually just having a special Glib::MainContext method to post a message to that context from another thread would be perfect, and there wouldn't be any confusion with the signal_idle/timeout() connect() calls.
Addendum: The method could take an std::function to completely avoid all the sigc++ trackable issues.
There are now thread-safe connect_once etc functions - see https://developer.gnome.org/glibmm/2.52/classGlib_1_1SignalIdle.html . It would be possible to change the signature to take a std::function object, since I think there is API/ABI breakage currently going on, but the recommended approach with glibmm-2.52 and earlier is to pass a lambda or the return value of std::bind, instead of using sigc::mem_fun, so as not to apply trackability when connect_once() is invoked by a worker thread. However sometimes trackability is what you want - just not when called by a worker thread.
(In reply to Chris Vine from comment #17) > There are now thread-safe connect_once etc functions - see > https://developer.gnome.org/glibmm/2.52/classGlib_1_1SignalIdle.html . Nice! Perhaps it's time to close this bug then. > However sometimes trackability is what you want - just not when called by a > worker thread. You are right, it would indeed be very useful to tie the slot's lifetime to the receiver object in the target thread. I'm a bit out of the loop -- is that actually possible now without violating thread safety?
Not it isn't possible, except where the thread calling connect_once() is also the thread in which the callback will execute. This can't be circumvented without rewriting libsigc++ (which wouldn't be a bad idea: trackability should be a property of the connection, not a property of the object to which the connection is made).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glibmm/issues/2.