GNOME Bugzilla – Bug 745013
GBinding not thread safe
Last modified: 2018-05-24 17:29:39 UTC
I simply pressed Ctrl+d to remove a mail and evolution crashed: Core was generated by `evolution'. Program terminated with signal SIGABRT, Aborted.
+ Trace 234701
Thread 36 (Thread 0x7fc3998c6940 (LWP 30961))
Thread 30 (Thread 0x7fc2ba7fc700 (LWP 26172)):
Created attachment 297641 [details] Full trace
Created attachment 297642 [details] Offending mail
Comment on attachment 297642 [details] Offending mail Umm... no, that is not the mail... I am searching still
Created attachment 297645 [details] mail.mbox
Thanks for a bug report. Looking into the full backtrace I see that Thread 2 and Thread 1 are both accessing the same GHashTable at once. Both are inserting into the hash table (modifying its content). It's GLib / GBinding issue, thus I move this there.
The GHashTable is retrieved using qdata, which should be thread safe, but the access inside the hash table itself is not thread safe. To be honest, we never said that bindings are thread safe; we could specify it into the documentation, but maybe we can do better.
Please do better. If you look into the full backtrace, on those two threads, then you'll see that no application has any real way of adding thread safety on its own. Maybe only if it'll avoid usage of GBinding all together or ..., I do not know, I do not want to sound silly. I just believe that the thread safety will be beneficial for any GLib users.
Looking at the issue some more, and it's not an entirely trivial issue that would be solved by locking the hash table when adding the binding data to the source and target. In the strictest sense, the issue is that two threads are trying to create a binding on the same instance; what happens is that: * we retrieve the set of bindings associated with an instance from its qdata * if there is no set, we create one and add it to the instance's qdata * we add the GBinding instance to the set We would need to "lock" the instance while we do this, because while retrieving the bindings set from the qdata and setting it are safe operations (the GData is accessed atomically or under a lock), multiple threads could try to add different GHashTable instances. We don't have any mechanism inside GObject to say "please, lock this instance while I do stuff", and even if we did, it would very likely interfere with custom property setter/getter code. Creating a binding inside a thread means that the property setter and getter on both the source and the target need to be thread safe, because, for instance, SYNC_CREATE will cause the property on the source instance to be retrieved, and some value to be set on the target instance. We may need something similar to GWeakRef associated to the GBinding instances.
Okay. I checked the related code in evolution and it was the only GBinding usage in that function, a direct usage, thus I replaced it with simple get&set of a property, which might be sufficient in this case. That was done with commit_36776ba in evo master (3.15.91+) [1]. It doesn't fix general issue with thread unsafe GBinding usage, only this concrete place in evolution. [1] https://git.gnome.org/browse/evolution/commit/?id=36776ba
(In reply to Milan Crha from comment #9) > That was done with commit_36776ba in evo master (3.15.91+) [1]. It doesn't > fix general issue with thread unsafe GBinding usage, only this concrete > place in evolution. I dived deeper into this and added thread safe variants of the g_object_bind_property*() functions into evolution-data-server and use them across the projects. The thread safety is added by a lock on the whole function call, which might help in this case too. Most of the places in evolution are related to widgets, thus it's all done in the main thread, but I wanted to be consistent, thus I replaced the calls also there. It would be still good to (somehow) add thread safety into GBinding itself, because my proxy functions do not cover binding removals.
Created attachment 304559 [details] [review] binding: Remove GObject data usage It isn't actually doing anything, instead it is being managed without actually being used. This has the result that GBinding is now more thread-safe.
Review of attachment 304559 [details] [review]: The idea behind storing the GBinding instances inside a GHashTable associated with the source/target GObject instance was to have API that let you query whether or not a GObject has bindings, and eventually access each binding to let you either query it or remove it. I never committed it because I didn't know if it would be used, and I didn't want to overcomplicate it. After 5 years nobody asked for it, so I guess we could simply ignore it, and remove it. It won't really make GBinding thread safe, though: we use g_object_weak_ref()/unref(), and the code would need to be switched to GWeakRef.
Comment on attachment 304559 [details] [review] binding: Remove GObject data usage Thanks for the crazy fast reviews!
(In reply to Garrett Regier from comment #15) > Comment on attachment 304559 [details] [review] [review] > binding: Remove GObject data usage > > Thanks for the crazy fast reviews! Could you push it into the stable version too, please? I've just got a crash report [1] from the functions you removed, thus it makes sense to cover it in stable as well. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1236293
-- 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/glib/issues/996.