Accounts Service always relies on language fallback if never set by the user

[Ricardo Salveti]

trusty and vivid SRU requests
=============================

[Impact]

Until a user has set the language or regional formats explicitly, accountsservice makes the system default values available. The code for 'calculating' the system defaults is expensive, and it's triggered frequently. The case in the original description of this bug report is one example of bad performance for this reason. Another example is a system with many users (see e.g. bug #1350393).

This upload adds a couple of static variables inside the function in question, to avoid that the expensive code is executed at each invocation. Under certain conditions this improves the performance significantly.

[Test Case]

Hmm.. There is no easy use case to reproduce the bug. The original description below gives a hint.

[Regression Potential]

On a multi-user system, if the system defaults in /etc/default/locale are changed, accountsservice will keep providing the old system default values until the system is rebooted. (Previously it took effect instantly.) I think the advantages with the proposed change outweigh this subtle change in behavior (which hardly anyone will notice anyway).

Can't think of anything besides that.

[Original description]

current build number: 169
device name: mako
channel: ubuntu-touch/devel-proposed
alias: ubuntu-touch/vivid-proposed
last update: 2015-04-12 20:38:14
version version: 169
version ubuntu: 20150412
version device: 20150210
version custom: 20150412

This causes a bad side effect when changing volume via indicator-sound, as that will cause a sync to accountsservice in order to sync the volume. Once that sync happens, it will request the user properties, and in case the user doesn't have a valid language at /var/lib/AccountsService/users/<user>, it will always rely on the fallback, which would be fine if calculating the fallback wasn't 't so cpu or i/o intensive (and that happens multiple times).

As a test, just flash latest vivid image on mako, don't set any language when the wizard shows up, run top and then change the volume by pressing volume up/down. This is what I see with mako:

 PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
2609 phablet 20 0 499660 121220 52688 S 7.6 6.5 0:41.69 unity8
5600 phablet 20 0 3676 1760 1288 R 6.0 0.1 0:00.19 language-option
1312 root 20 0 211532 15572 11344 S 1.9 0.8 0:07.25 unity-system-co
1316 phablet 20 0 36532 3792 2928 S 1.3 0.2 0:01.66 accounts-daemon

And the reason why:
src/user.c
...
static void
user_get_property (GObject *object,
                   guint param_id,
                   GValue *value,
                   GParamSpec *pspec)
{
        User *user = USER (object);
...
        case PROP_LANGUAGE:
                if (user->language)
                        g_value_set_string (value, user->language);
                else
                        g_value_set_string (value, user_get_fallback_value (user, "Language"));
                break;
        case PROP_FORMATS_LOCALE:
                if (user->formats_locale)
                        g_value_set_string (value, user->formats_locale);
                else
                        g_value_set_string (value, user_get_fallback_value (user, "FormatsLocale"));
                break;

user_set_property never gets called unless the user changes the system language from system-settings or wizard.

Once you change the language, it will set a valid language at /var/lib/AccountsService/users/<user>, causing this behavior to stop.

Another bad side effect of this issue is that it takes quite a while for accountsservice to reply back to indicator-sound when the sync happens, possibly causing sync aborts (as indicator-sound only waits 1 second before triggering another sync).

Some possible ways to fix this issue:
1) Make wizard to set language even when the selected language is already the default one;
2) Change accountsservice to save the fallback value at the first time it gets that from the system;

[Michał Sawicz (saviq)]

Not sure what unity8 could do here?

[Michał Sawicz (saviq)]

OK so the unity8/wizard part seems to be that the language isn't being stored in AS if it's not changed from default. This could very well be libubuntu-system-settings then.

[Gunnar Hjalmarsson (gunnarhj)]

Hi Michal,

Please note that the problem isn't Unity 8 specific. I added that user_get_fallback_value() function a few years ago (admittedly it's a little 'heavy'), and the purpose was to get the start value right in various GUIs for setting language/locale, both in Ubuntu and the flavours. Even if I'm not quite sure yet, I think the problem is best fixed in the accountsservice package.

[Gunnar Hjalmarsson (gunnarhj)]

I wrote a patch which makes accountsservice set the Language and FormatsLocale properties automatically as soon as possible and updates ~/.pam_environment accordingly. I built a test version of accountsservice in my PPA at

https://launchpad.net/~gunnarhj/+archive/ubuntu/accountsservice-test

@Ricardo:
Can you possibly test this version on the phone, and confirm that the fix addresses the issue?

@Sebastien:
This means that the behaviour (behind the scenes) changes for everyone. Language and regional formats are set for every user, based on /etc/default/locale, before the users have touched a GUI for the purpose. I made some code rearrangement to achieve it, and the most 'hackish' part is that it happens in the user_get_property() function.

I'd appreciate your eyes on it before uploading. Especially:
1. Do you see any drawback with enforcing user settings of language and
   regional formats?
2. Is this an acceptable way to do it?

[Sebastien Bacher (seb128)]

@Gunnar, I'm unsure to understand enough about the issue to comment on the solution. Having some config automatically writen for users seems suboptimal, especially when the config is identic to the system default. Can't we just bail out from doing work in those cases?

[Gunnar Hjalmarsson (gunnarhj)]

Thanks for your comment, Sebastien!

Let me say a few words about the reason for involving the user_get_property() function in the first place.

In addition to generating lists of available options, the GUIs for setting language and regional formats asks accountsservice about the current value. Packages which rely on accountsservice in this respect are:

- language-selector
- unity-control-center (for User Accounts)
- gnome-control-center (for both Region & Language and User Accounts)
- lightdm (for lightdm-gtk-greeter)
- ubuntu-system-settings

So far, as long as the user has not set some other value but the system default, and since accountsservice does not store system defaults, accountsservice has generated the value on the fly. The code for doing so is somewhat expensive, and - as mentioned in the bug description - user_get_property() is called frequently.

In the light of this, changing it so the user config is saved (based on system default) is a way to improve the efficiency. If we would take the other route, and let accountsservice respond with the empty string, we would mess it up in several places.

Please note that I'm not asking for a detailed code review. I merely ask if you are strongly against the approach (saving user settings based on system defaults). After this explanation, I hope you are not. :)

[Gunnar Hjalmarsson (gunnarhj)]

The proposed solution might also help address the issue reported at bug #1350393.

[Gunnar Hjalmarsson (gunnarhj)]

I consulted with Sebastien Bacher and Lars Uebernickel at #ubuntu-desktop, and we concluded that it's better to cache the values in memory compared to silently saving them as user settings. Accordingly I have uploaded a patch which adds two static variables inside user_get_fallback_value() to prevent execution of expensive code at each invocation.

[Launchpad Janitor (janitor)]

This bug was fixed in the package accountsservice - 0.6.37-1ubuntu11

---------------
accountsservice (0.6.37-1ubuntu11) wily; urgency=medium

  * debian/patches/0010-set-language.patch:
    Use static variables inside user_get_fallback_value() to prevent
    execution of expensive code at each invocation (LP: #1443178).

 -- Gunnar Hjalmarsson <email address hidden> Tue, 12 May 2015 16:17:00 +0200

[Chris J Arges (arges)]

Hello Ricardo, or anyone else affected,

Accepted accountsservice into trusty-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/accountsservice/0.6.35-0ubuntu7.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Chris J Arges (arges)]

Hello Ricardo, or anyone else affected,

Accepted accountsservice into vivid-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/accountsservice/0.6.37-1ubuntu10.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

[Gunnar Hjalmarsson (gunnarhj)]

I have successfully installed and run both accountsservice 0.6.35-0ubuntu7.2 from trusty-proposed and accountsservice 0.6.37-1ubuntu10.1 from vivid-proposed. Querying accountsservice for language and regional formats seems to work as expected, and even if I haven't measured it, the desired performance improvement requested by the bug reporter has most likely been achieved.

[Launchpad Janitor (janitor)]

This bug was fixed in the package accountsservice - 0.6.37-1ubuntu10.1

---------------
accountsservice (0.6.37-1ubuntu10.1) vivid-proposed; urgency=medium

  * debian/patches/0010-set-language.patch:
    Use static variables inside user_get_fallback_value() to prevent
    execution of expensive code at each invocation (LP: #1443178).

 -- Gunnar Hjalmarsson <email address hidden> Tue, 12 May 2015 17:44:00 +0200

[Brian Murray (brian-murray)]

The verification of the Stable Release Update for accountsservice has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package accountsservice - 0.6.35-0ubuntu7.2

---------------
accountsservice (0.6.35-0ubuntu7.2) trusty-proposed; urgency=medium

  * debian/patches/0010-set-language.patch:
    Use static variables inside user_get_fallback_value() to prevent
    execution of expensive code at each invocation (LP: #1443178).

 -- Gunnar Hjalmarsson <email address hidden> Tue, 12 May 2015 18:30:00 +0200