GNOME Bugzilla – Bug 396963
Add Glib::signal_idle().connect_once()
Last modified: 2013-02-20 18:04:59 UTC
New API proposal: Add a connect_once() method to the idle and timeout signals in glibmm which takes a sigc::slot<void> and doesn't return a sigc::connection object. This new API would simplify a common use case of these signals. Currently, if one connects a method not defined in one's own source code to the idle signal, it is necessary to either write a wrapper function or use sigc::bind_return(). Beyond the simplification, this new API call would ensure that the thread-safety problem of bug #396958 simply cannot occur, since the complexities of returning a sigc::connection are avoided.
Yes, this would be useful.
A patch would be welcome.
A patch would still be welcome.
Created attachment 119184 [details] [review] idle and timeout connect_once functions Comments and formatting are probably off but this adds the requested functions.
Bah, forgot to mention, the patch is against 2.16.4.
hm, I guess it's too late for this since we missed the 2.18.0 release. Sorry about that. We can add it for 2.20 though.
Thanks, I've committed this as r750. Which means that I'd better branch for 2.18 I suppose...
http://git.gnome.org/cgit/glibmm/tree/glib/glibmm/main.cc#n313 This implementation of connect_once() fails to solve the problem for which it was originally proposed as a solution. void SignalTimeout::connect_once(const sigc::slot<void>& slot, unsigned int interval, int priority) { connect(sigc::bind_return(slot, false), interval, priority); } This has exactly the same problem as before, just moved to the library instead of the application. Reopening.
Created attachment 210311 [details] [review] patch: Make SignalTimeout,SignalIdle::connect_once() more thread safe. Is this better? Even good enough? No sigc::connection is created by the connect_once() methods. SourceConnectionNode::install() is called before g_source_unref(), when the SourceConnectionNode object still exists.
Some changes in that patch seem like they could be separated out.
Do you mean that the changes in examples/thread/dispatcher.cc and dispatcher2.cc should be in a separate patch? Or even two separate patches, since there are two unrelated changes in dispatcher2.cc? I can split the patch into 2 or 3 patches, but first I'd like to give Daniel Elstner and others some time to comment on my changes of glib/glibmm/main.cc. Almost 3 years ago Daniel disliked the present version of the connect_once() methods. Daniel, are you still interested in this matter?
(In reply to comment #11) > Do you mean that the changes in examples/thread/dispatcher.cc and > dispatcher2.cc should be in a separate patch? Or even two separate patches, > since there are two unrelated changes in dispatcher2.cc? I think so, yes. If there is unrelated stuff that you can commit now, you should probably do that. > I can split the patch into 2 or 3 patches, but first I'd like to give Daniel > Elstner and others some time to comment on my changes of glib/glibmm/main.cc. > Almost 3 years ago Daniel disliked the present version of the connect_once() > methods. I doubt you will get a response from Daniel. But anyway, my point is that it's easier for anyone to review a smaller patch.
Created attachment 210368 [details] [review] patch: Make SignalTimeout,SignalIdle::connect_once() more thread safe. I've pushed a separate commit with the changes in examples/thread/dispatcher.cc and dispatcher2.cc. Here's a patch with only the changes in glib/glibmm/main.cc. If no one has objected within 2 to 3 weeks, I'll probably push it. It's definitely more thread safe than the present version of the connect_once() methods, but it's more or less impossible to get 100% thread safety with slots involved. sigc::internal::slot_rep derives from sigc::trackable, which is not thread safe.
Pushed. Thanks.
I don't think glibmm_signal_connect_once() is thread safe unless the call to conn_node->install(source) is put before the call to g_source_attach(source, context), so that it is protected by the main loop lock.
"so that it is protected by the main loop lock". I should elucidate that this is not to prevent concurrent access (which it wouldn't) but to enforce memory ordering/visibility. See also http://mail.gnome.org/archives/gtkmm-list/2007-January/msg00145.html for further detail. An alternative is to put LOCK_CONTEXT()/UNLOCK_CONTEXT() calls around conn_node->install(source), but that causes a second lock/unlock operation just to enforce visibility.
Chris, do you want glibmm_signal_connect_once() changed to the following version? I can change it like that. Is LOCK_CONTEXT()/UNLOCK_CONTEXT() really an alternative? Aren't those macros, and the GMainContext::mutex they use, only available in glib/gmain.c? static void glibmm_signal_connect_once(const sigc::slot<void>& slot, int priority, GSource* source, GMainContext* context) { SourceConnectionNode* const conn_node = new SourceConnectionNode(slot); if (priority != G_PRIORITY_DEFAULT) g_source_set_priority(source, priority); g_source_set_callback( source, &glibmm_source_callback_once, conn_node, &SourceConnectionNode::destroy_notify_callback); conn_node->install(source); g_source_attach(source, context); g_source_unref(source); }
Yes, that's what I meant. This guarantees that everything above g_source_attach() is synchronized, and g_source_unref() is intrinsically thread safe. There was a report which I cannot now find on the mailing list archive, by someone who did stress tests, that threading errors with idle dispatches practically disappeared with this change. I put the remaining errors down to the fact that the sigc::connection return value was not thread safe. He did offer to rerun the tests when that aspect was dealt with, but as I say I cannot now find out who it was and it was 4 or 5 years ago: possibly he sent me a private e-mail. It would be nice if further stress tests could be carried out, but this change deals with all the remaining obvious synchronization issues. You are right that I am wrong about LOCK_CONTEXT()/UNLOCK_CONTEXT(). Unfortunately it is inaccessible, as is the context mutex member. If this change is to be advertised in the documentation, it ought to be mentioned that attaching a slot representing a non-static method of an object deriving from sigc::trackable is still thread unsafe.
I have pushed the change suggested by Chris Vine. http://git.gnome.org/browse/glibmm/commit/?id=ac40deaa6e02797fb228e960dfff8c466f967c32
(In reply to comment #18) > If this change is to be advertised in the documentation, it ought to be > mentioned that attaching a slot representing a non-static method of an object > deriving from sigc::trackable is still thread unsafe. Done, at last. http://git.gnome.org/browse/glibmm/commit/?id=6cba8b3271b5c5def5a386b076420615c78fe4b7