GNOME Bugzilla – Bug 733791
GSettings: delay backend subscription
Last modified: 2018-05-24 16:51:45 UTC
GSettings objects begin watching for changes as soon as they are created in order that they can emit the "changed" signal. In the case of dconf, if we want to be able to emit the changed signal, we need to go on the bus and add some match rules. This requires creating the dconf helper thread and also requires initialising GDBus (which creates another thread). Some users of GSettings are never interested in the "changed" signal. One of these users is the glib-networking code that gets run every time a new network connection is created. Some users are reporting that they are annoyed that simply establishing a network connection would spawn two extra threads and create a D-Bus connection. In order to avoid doing unnecessary work for these simple uses, delay the subscription until we know that we will actually need to do it. We do this in a simple way, using a simple argument: in order for the user to care that a value changed then they must have: 1) watched for a change signal; and then 2) actually read a value If the user didn't actually read a value then they cannot possibly be interested in if the value changed or not (since they never knew the old value to begin with and therefore would be unable to observe that it ever changed, since they have nothing to compare the new value with). This really is a behaviour change, however, and it does impact at least one user: the 'monitor' functionality of the GSettings commandline tool, which is interested in reporting changes without ever having known the original values. We add a workaround to the commandline tool in order to ensure that it continues to function properly. It's also possible to argue that it is completely valid to have read a value and _then_ established a change signal connection under the (correct) assumption that it would not have been possible to miss a change signal by virtue of not having returned to the mainloop. Although this argument is true, this pattern is extremely non-idiomatic, and the problem is easily avoided by doing things in the usual order. We never really talked about change notification in the overview documentation for GSettings, so it seems like now is a good time to add some discussion, including the new rules for when one can expect change signals to be emitted.
Created attachment 281780 [details] [review] GSettings: delay backend subscription
Review of attachment 281780 [details] [review]: Interesting. Thanks.
Attachment 281780 [details] pushed as 8ff5668 - GSettings: delay backend subscription
*** Bug 623403 has been marked as a duplicate of this bug. ***
This made Alberto sad. Let's see if we can do a better job by fixing bug 380581.
Hello, Unfortunately commit 8ff5668 also gave me a :| as well, as this broke the registry GSettings backend (the one used on Windows). This caused GTK+3.x demos that retrieved GSettings items crash due to an Access Violation. I opened bug 740413 for that. With blessings, thank you!
(In reply to comment #0) > > If the user didn't actually read a value then they cannot possibly be > interested in if the value changed or not (since they never knew the old > value to begin with and therefore would be unable to observe that it > ever changed, since they have nothing to compare the new value with). I don't think this is true > This really is a behaviour change, however, and it does impact at least > one user: the 'monitor' functionality of the GSettings commandline tool, > which is interested in reporting changes without ever having known the > original values. We add a workaround to the commandline tool in order > to ensure that it continues to function properly. ...and you actually found a counterexample yourself a few lines later!
(In reply to comment #7) > I don't think this is true Reviews welcome on bug 380581 in order to facilitate my abandoning my weak arguments :)
A little late... but this troubles me a bit... so to get change signals, you have to really want it now, and you have to perform multiple steps in a particular order, or else there's no guarantee you'll get those callbacks. What about the many many programs out there that group _connects together for readability - this first section if my _init function is for reading my settings and initializing widgets or whatever, and at the end let's connect all our callbacks at once (it looks more organized, particularly when you look at _dispose or _finalize and the signal disconnects are all grouped together as well. Also, and perhaps a bit of over-caution/superstition on my part, I tend to want to set up my object *first* before listening for changes that may affect it, to make sure things are in a good state to receive those changes (which could occur outside the current thread.) I'd prefer not to add an extra superfluous read just to say 'Yes, I really really want this callback to work.' Couldn't we keep the old behavior as default, and offer a property that can be set on GSettings to enable the new behavior for the cases where it is an issue? It just seems that this new behavior opens huge regression potential for unsuspecting programs, particularly if the developer doesn't stay on top of glib changes. Thanks
I'll add that I don't see any reason why you have to wait for the user to read a value before starting to listen. If someone connects to the signal, then I'm pretty sure they want to connect. This additional check is completely unintuitive and prone to causing trouble.
Okay sorry I see that GObject doesn't provide a way to know when users are connecting to a signal. But I'll concur with mtwebster that this behaviour should be opt-in instead of default since it is not intuitive and not everyone is going to read through the long gsettings documentation (and it breaks backwards compatibility).
One last argument.. (sorry!) Everything we do here is about events and responses to those events. I know if I want to be notified something has changed, I connect to the appropriate signal and can count on being told about it. I can look at code I'm completely unfamiliar with, but if it uses GObject, I will have an immediate partial understanding, and a huge head start on figuring out what's going on. I can trust in this behavior - I can take this stuff to the bank. But now we've introduced a case where some of our assumptions are potentially invalid, depending potentially on how we've organized our program, or whether we made sure we were *really* interested in this signal. If I'm the guy coming into a project, seeing unfamiliar code, I can no longer assume that if I connect to a signal, I can count on my callback being run. I know I'm drawing it out to a slight extreme, but it just doesn't "fit." I'll admit I only read through the first post here for a description of the reason for this change, but if at all possible I think this should be some new api or subclass of GSettings - GSettingsStatic maybe... without change event handling, you can only set or get (or define that by a constructor argument.) Ok ok I'll shut up now - thank you!
Correction to above suggestion - I suppose you'd have to make GSettings the subclass or else you're inheriting signals (and the assumption of inherited behavior behaving), or simply create a new class for this simpler behavior.
It's a very bad decision. After connected signals and run g_main_loop, We should be able received signals. If this convention has been broken, what else can be guaranteed? Especially, there hasn't any explain in document and gsettings monitor is workable! The gsettings can be used to share configuration datas. This meanings some programs many only do actions when configuration has been changed. There has many "gsettings monitor" code style, we have at least two weird bug caused by this. And thanks you guys has explained this in commit messages. But many application developer can't find this.
Hi, Sorry to comment so late on this. This change can potentially break any application using gsettings in very random ways. Nobody is ever going to review all that code in so many apps so if we let it in, we could be exposed to regressions in many places and it could take years to match feedback from novice users with the actual cause of the issue. We'll have no choice but to patch Glib to avoid that. We're also reluctant to "fix" this downstream and in isolation for the software we maintain while thousands of other packages might potentially be affected as well. We'll be directing bug reports here instead since this is indeed a regression in Glib. I hope the case fixed by Ryan can be fixed some other way and without impacting the existing functionality (we rightly assume connect() to always actually connect, if a particular case needs a different behaviour why not introduce a new function in the interface?).
(In reply to Clement Lefebvre from comment #15) > I hope the case fixed by Ryan can be fixed some other way and without > impacting the existing functionality (we rightly assume connect() to always > actually connect, if a particular case needs a different behaviour why not > introduce a new function in the interface?). The problem that we are trying to solve here is that every g_settings_new() call causes quite a bit of overhead, including installing dbus filters, etc. Adding new API is not an effective way to address that.
(In reply to Matthias Clasen from comment #16) > The problem that we are trying to solve here is that every g_settings_new() > call causes quite a bit of overhead, including installing dbus filters, etc. > Adding new API is not an effective way to address that. How about a new constructor function that doesn't connect? Say g_settings_new_static()?
This overhead is incurred at g_settings_backend_subscribe() which tells the backend we're interested in changes. Why not an opt-out constructor (as mentioned, new_static()) which skips the subscription? Pros: - simpler change than the original patch - backward compatible - opt-in behavior Cons: - nothing is preventing anything from still connecting to these signals - they'll just never do anything. Though, if you opted in to this behavior, you're already aware of this. - is a bit hackish since we now have a class with two different types of behavior possible (though still way better than having a class that doesn't obey the accepted GObject principles..)
Created attachment 304877 [details] [review] rough/incomplete patch proposal
Comment on attachment 304877 [details] [review] rough/incomplete patch proposal Note this diff is with the original patch (and a subsequent fix) already reverted
See comment 16
I am under the impression that a majority of the use case of gsettings is interested in listening to value changes, and most of the time the overhead caused by listening is insignificant compared to everything else that is going on. In performance-critical situations, they can opt in to use the new constructor. I believe this is a superior solution than breaking every application using gsettings out there.
Okay, what about a different approach.. you're now using g_settings_get as a way of knowing someone's interested in these signals. What if GObject had a new overridable method could notify that someone is interested? if (class->notify_handler_added) class->notify_handler_added()). Then, the actual connecting that signal could trigger the backend subscription, instead of requiring an additional step from the client. This has the potential to be useful in many other situations I would imagine.
(In reply to dalcde from comment #22) > I am under the impression that a majority of the use case of gsettings is > interested in listening to value changes, and most of the time the overhead > caused by listening is insignificant compared to everything else that is > going on. In performance-critical situations, they can opt in to use the new > constructor. I believe this is a superior solution than breaking every > application using gsettings out there. I agree with this. We can clearly change the glib-networking code to use this. (Although of course - any time one is retrieving a gsetting but *not* listening for changes, you have to ask "why not"? In this case of course I doubt users would expect existing network connections to break and restart when the proxy is changed...but one could imagine a future in which you can say your connection is stateless and handle it)
(In reply to Michael Webster from comment #23) > Okay, what about a different approach.. you're now using g_settings_get as a > way of knowing someone's interested in these signals. What if GObject had a > new overridable method could notify that someone is interested? if > (class->notify_handler_added) class->notify_handler_added()). It's not trivial (otherwise we would have already had it); there are various subtle race conditions lurking there, alongside a bunch of locking nightmares. It's not a simple job. See also: bug 635054
(In reply to Matthias Clasen from comment #16) > (In reply to Clement Lefebvre from comment #15) > > > I hope the case fixed by Ryan can be fixed some other way and without > > impacting the existing functionality (we rightly assume connect() to always > > actually connect, if a particular case needs a different behaviour why not > > introduce a new function in the interface?). > > The problem that we are trying to solve here is that every g_settings_new() > call causes quite a bit of overhead, including installing dbus filters, etc. > Adding new API is not an effective way to address that. I talked to the MATE Gentoo maintainer today. Their settings daemon no longer react to theme changes. On our sides I think we've seen 5 different issues in Cinnamon alone. The problem you solved is a problem we never heard users complain about. Yet the solution brought here creates real problems all over the place. It's easy to workaround this glib regression by artificially reading gsettings values we don't need, or by re-factoring code slightly to meet the demands of a now more-fussy and less-reliable API. I'm worried this just contributes to hiding a real issue here though. Once we fixed the most obvious breakages caused in MATE and Cinnamon, who's to say how long it might take until user feedback highlights other affected areas? and what about other projects where maintainers/developers are less active or less prone to understand the link between what looks like an application bug and this regression in glib? It doesn't seem worth the gain at all. Would you considering reverting this change until you find a way to fix that overhead without creating such regressions? We've been refusing issues and pull requests for workarounds so far in the hope that this gets fixed upstream, and recommending maintainers patch glib instead to have a functional system until a resolution is found.
I've been ignored for more than a month, so I guess it's time for me to be pragmatic. In Cinnamon we'll start accepting PRs and implement changes to make gsettings' connect() work again for users of other distributions. In Mint, if we want to be rigorous, we should patch and revert your change to avoid this regression hitting software that hasn't been "fixed" a posteriori. I guess we'll see how this affects other distributions first. Long term, you need to respect backward compatibility and existing code. If you want to change something that breaks the current ecosystem, then please bump the API/lib/version of what you're doing so that it doesn't impact what's already there. We're faced with numerous critical bugs, you're merely optimizing things and yet you don't seem to care about the regressions this is causing. Downstream we don't have much choice. Everything has to work. We can't rely on something which design doesn't take us into consideration. What are we to do when you don't react to feedback?
My preference is to revert and add a new API.
I concur
I've reverted these changes for now. We can try again next cycle, either with snooping on signal connections, or by adding explicit notification-free api.
+1 on the signal thing. No extra API here please. It's worth noting that I plan for GSettings to get a bit more clever about change notifications in the future and it will very much be in the direction of this patch. For example, when we do 'dconf update' and "/" changes, we emit changes for all of the things, even if they didn't actually change.... This "/ changed" business also happens if we race the async establishment of a watch against a change happening to the database, since we have no idea what the change was. This sometimes happens on login (due to unfortunate login-time writes that we can never seem to get rid of for good). In this case we'd only emit the change signal if the change actually impacted one of the watched keys.
https://github.com/GNOME/glib/commit/7fff264777ac9869ff347dd2bb02304e11d83a20 https://github.com/GNOME/glib/commit/1dec512a66fddfd8b4b265231b00d4f918b16cef Many thanks Matthias & Colin. Desrt, we noticed random change signals during the login sequence for values which hadn't changed. To prevent this from causing issues and/or crashing components, we're very careful not to connect to changes "too early" in the session :)
-- 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/906.