Merge lp:~xavi-garcia-mena/indicator-sound/restart-dbus-error into lp:indicator-sound/15.10

[Xavi Garcia]

Added some time before exiting the indicator if we've got an error owning the dbus name. This lets dbus system to fully start

Added some time before exiting the indicator if we've got an error owning the dbus name. This lets dbus system to fully start

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:508
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/285/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/61
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/61
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/61/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/285/rebuild

review: Approve (continuous-integration)

[lp:~xavi-garcia-mena/indicator-sound/restart-dbus-error]

509. By Xavi Garcia on 2015-11-09

Added a retry mechanism to try to own the dbus name

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:509
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/62
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/62
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/62/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/286/rebuild

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

No. Please don't fix race conditions with sleep().

review: Disapprove

[Xavi Garcia (xavi-garcia-mena)]

Thanks for the review, Lars.

I don't like the sleep either, but if the indicator is starting while dbus is not ready yet I guess we can only implement a wait/retry mechanism waiting to be ready or go down to the real root of the problem and see why is this happening. Maybe upstart is starting the indicator too early.

From the indicator point of view I don't see another way of doing this, though.

[Lars Karlitski (larsu)]

> I don't like the sleep either, but if the indicator is starting while dbus is
> not ready yet I guess we can only implement a wait/retry mechanism waiting to
> be ready or go down to the real root of the problem and see why is this
> happening. Maybe upstart is starting the indicator too early.

Yes, please find the real cause for this issue. That's probably not easy, but no desktop component can deal with D-Bus not being available, and I don't think patching all of them like this is desirable. There's already too many pitfalls to watch out for when using D-Bus. Let's not add another one.

Do you have a specific bug that you're fixing with patch? If so, please link it from this Merge Proposal.

[Xavi Garcia (xavi-garcia-mena)]

sure, bug linked.

[lp:~xavi-garcia-mena/indicator-sound/restart-dbus-error]

510. By Xavi Garcia on 2015-12-09

Modified time to wait

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:510
http://jenkins.qa.ubuntu.com/job/indicator-sound-ci/292/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-amd64-ci/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/68
        deb: http://jenkins.qa.ubuntu.com/job/indicator-sound-wily-armhf-ci/68/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-sound-ci/292/rebuild

review: Approve (continuous-integration)

[Xavi Garcia (xavi-garcia-mena)]

Although the approach in this branch is not ideal I'm implementing the workaround like this to wait for dbus session connection to be ready.

This might happen to any indicator, but is more easy to recreate for the sound indicator as the very first thing the indicator does is to try to acquire its dbus name.

I've opened a separated bug for this:
https://bugs.launchpad.net/upstart/+bug/1525183 in which the root issue should be fixed.

I've also like to note that the issue will be probably fixed when we move to systemd.
The idea would be to rollback this branch when the switch to systemd is done.

[Lars Karlitski (larsu)]

Even if you wanted to merge this, I don't see how this patch fixes any problem.

If the connection is really not there (which I highly doubt, but might be something related to what Pete was saying in the linked bug), indicator-sound should simply exit, because the on_name_lost() callback is called in that case with a NULL connection. I just verified that it does this by unsetting DBUS_SESSION_BUSA_ADRESS. Upstart should automatically respawn the process after some time.

Do you know who sends the SIGTERM to the process?

What's the stack trace of the critical that is pasted in the linked bug (run it with G_DEBUG=fatal-criticals)?

[Xavi Garcia (xavi-garcia-mena)]

Hi Lars,

the process exists itself, no other process sends SIGTERM.
It just stops the loop and exists.

I saw upstart tries to respawn, but only 3 times. After 3 times the result is a phone with no sound indicator.

Sadly I only managed to recreate this with the Nexus 4 and mine is not booting now due to battery issues.

[Lars Karlitski (larsu)]

> the process exists itself, no other process sends SIGTERM.
> It just stops the loop and exists.

Ah sorry I misread the patch as adding those lines to the sigterm handler. Where does the warning about g_object_unref() come from then, though?

> I saw upstart tries to respawn, but only 3 times. After 3 times the result is
> a phone with no sound indicator.

You can change that (and the timing) in the upstart conf file `data/indicator-sound.conf.in`. (I can't believe I'm suggesting that you do this though ;) )

Please talk to Pete about how they fixed the issue in upstart. Seems other projects have this problem as well. I don't see how it makes sense to fix this only here.

[Xavi Garcia (xavi-garcia-mena)]

I already chatted with Pete about it.

It seems the problem was that upstart was not setting the DBUS env variables properly.
I checked that already and everything seems to be OK...

I know this is not a real solution, but meanwhile we move to systemd or fix the root issue in upstart I think it's best have a workaround that, at least, doesn't force the user to reboot the phone to get sound indicator.

Thanks for the "suggestion" on the upstart config file, I guess changing that would be enough.
I will go to a store this afternoon to get a screwdriver to remove my Nexus 4 battery and will give it a try.

Unmerged revisions

510. By Xavi Garcia on 2015-12-09

Modified time to wait

509. By Xavi Garcia on 2015-11-09

Added a retry mechanism to try to own the dbus name

508. By Xavi Garcia on 2015-11-04

Added some time before exiting the indicator if we've got an error owning the dbus name. This lets dbus system to fully start

=== modified file 'src/main.c'
--- src/main.c	2015-03-10 19:16:26 +0000
+++ src/main.c	2015-12-09 13:36:30 +0000
@@ -14,6 +14,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <unistd.h>
+
 #include <glib.h>
 #include <locale.h>
 #include <libnotify/notify.h>
@@ -22,6 +24,8 @@
 #include "config.h"
 
 static IndicatorSoundService * service = NULL;
+static int DBUS_OWN_NAME_RETRIES = 30;
+static GMainLoop * loop = NULL;
 
 static gboolean
 sigterm_handler (gpointer data)
@@ -37,7 +41,16 @@
              gpointer user_data)
 {
 	g_warning("Name lost or unable to acquire bus: %s", name);
-	g_main_loop_quit((GMainLoop *)user_data);
+	if (DBUS_OWN_NAME_RETRIES-- == 0) {
+		g_main_loop_quit((GMainLoop *)user_data);
+	} else {
+		// we wait 10 milliseconds to let the system be completely up.
+		// sometimes the indicator starts while dbus seems to be
+		// still starting and if upstart is fast enough restarting
+		// the indicator it may fail to start in the 30 upstart retries.
+		usleep(10000);
+		try_bus_own_name();
+	}
 }
 
 static void
@@ -66,11 +79,22 @@
 	g_clear_object(&accounts);
 }
 
+void
+try_bus_own_name()
+{
+	g_bus_own_name(G_BUS_TYPE_SESSION,
+		"com.canonical.indicator.sound",
+		G_BUS_NAME_OWNER_FLAGS_NONE,
+		on_bus_acquired,
+		NULL, /* name acquired */
+		on_name_lost,
+		loop,
+		NULL);
+}
+
 int
 main (int argc, char ** argv)
 {
-	GMainLoop * loop = NULL;
-
 	bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8");
 	setlocale (LC_ALL, "");
 	bindtextdomain (GETTEXT_PACKAGE, GNOMELOCALEDIR);
@@ -83,14 +107,7 @@
 	/* Initialize libnotify */
 	notify_init ("indicator-sound");
 
-	g_bus_own_name(G_BUS_TYPE_SESSION,
-		"com.canonical.indicator.sound",
-		G_BUS_NAME_OWNER_FLAGS_NONE,
-		on_bus_acquired,
-		NULL, /* name acquired */
-		on_name_lost,
-		loop,
-		NULL);
+	try_bus_own_name();
 
 	g_main_loop_run(loop);