Merge lp:~albaguirre/media-hub/fix-1371454 into lp:media-hub

[Alberto Aguirre]

Fix self deadlock when clients disconnect from media::Player

When receiving a media::PlayerImplementation on_client_disconnected signal, avoid calling
remove_player_for_key within the same context as the player object may be deleted (which owns
the signal object whose destructor waits until all slots are dispatched).

Fix self deadlock when clients disconnect from media::Player

When receiving a media::PlayerImplementation on_client_disconnected signal, avoid calling
remove_player_for_key within the same context as the player object may be deleted (which owns
the signal object whose destructor waits until all slots are dispatched).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:68
http://jenkins.qa.ubuntu.com/job/media-hub-ci/132/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-amd64-ci/74
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/73
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/73/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-i386-ci/72

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/132/rebuild

review: Approve (continuous-integration)

[Leo Arias (elopio)]

Is there a way to add a regression test for this fix?
If not, is there a manual sequence of steps that would expose it that can be added to the test plan so it's less likely that it will regress?

The test suite in unity exposes the issue, but by the time the unity test suite runs, the feedback comes too late.

[lp:~albaguirre/media-hub/fix-1371454]

69. By Alberto Aguirre on 2014-09-25

simplify weak_ptr assignment; document issue

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:69
http://jenkins.qa.ubuntu.com/job/media-hub-ci/133/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-amd64-ci/75
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/74
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-armhf-ci/74/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-utopic-i386-ci/73/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/133/rebuild

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

> Is there a way to add a regression test for this fix?
> If not, is there a manual sequence of steps that would expose it that can be
> added to the test plan so it's less likely that it will regress?
>
> The test suite in unity exposes the issue, but by the time the unity test
> suite runs, the feedback comes too late.

Yes ideally should be part of the acceptance tests...but I see all those are disabled here...Not sure why.

Manually you could run this stress test:
http://pastebin.ubuntu.com/8423451

It will stop playing video after the 17 iteration (since all 16 binder threads are exhausted).

[Thomas Voß (thomas-voss)]

Thanks for the change. It is fine to land to address the immediate issues. However:

  http://paste.ubuntu.com/8424830/

is an alternate way of postponing execution without the need to start up a new thread. Would be great if you could adjust this MP to follow the pastebin example. Thanks :)

review: Approve

[Alberto Aguirre (albaguirre)]

> Thanks for the change. It is fine to land to address the immediate issues.
> However:
>
> http://paste.ubuntu.com/8424830/
>
> is an alternate way of postponing execution without the need to start up a new
> thread. Would be great if you could adjust this MP to follow the pastebin
> example. Thanks :)

OK, let me try.

[Leo Arias (elopio)]

> Yes ideally should be part of the acceptance tests...but I see all those are
> disabled here...Not sure why.
>
> Manually you could run this stress test:
> http://pastebin.ubuntu.com/8423451
>
> It will stop playing video after the 17 iteration (since all 16 binder threads
> are exhausted).

Great :) When you are ready to land it, please add that test to https://wiki.ubuntu.com/Process/Merges/TestPlan/media-hub
I will make a note to check what's going on with the disabled tests next week. Thanks.

[lp:~albaguirre/media-hub/fix-1371454]

70. By Alberto Aguirre on 2014-09-25

Use io_service instead of launching a thread

71. By Alberto Aguirre on 2014-09-25

remove empty line

[Jim Hodapp (jhodapp)]

Looks good, thanks.

review: Approve (code)

=== modified file 'src/core/media/service_implementation.cpp'
--- src/core/media/service_implementation.cpp	2014-09-09 21:27:29 +0000
+++ src/core/media/service_implementation.cpp	2014-09-25 17:23:52 +0000
@@ -23,6 +23,8 @@
 #include "player_configuration.h"
 #include "player_implementation.h"
 
+#include <boost/asio.hpp>
+
 #include <cstdint>
 #include <map>
 #include <memory>
@@ -35,10 +37,11 @@
 struct media::ServiceImplementation::Private
 {
     Private()
-        : resume_key(std::numeric_limits<std::uint32_t>::max())
+        : resume_key(std::numeric_limits<std::uint32_t>::max()),
+          keep_alive(io_service)
     {
         bus = std::shared_ptr<dbus::Bus>(new dbus::Bus(core::dbus::WellKnownBus::session));
-        bus->install_executor(dbus::asio::make_executor(bus));
+        bus->install_executor(dbus::asio::make_executor(bus, io_service));
         worker = std::move(std::thread([this]()
         {
             bus->run();
@@ -65,6 +68,8 @@
     media::Player::PlayerKey resume_key;
     std::thread worker;
     dbus::Bus::Ptr bus;
+    boost::asio::io_service io_service;
+    boost::asio::io_service::work keep_alive;
     std::shared_ptr<dbus::Object> indicator_power_session;
     std::shared_ptr<core::dbus::Property<core::IndicatorPower::PowerLevel>> power_level;
     std::shared_ptr<core::dbus::Property<core::IndicatorPower::IsWarning>> is_warning;
@@ -104,7 +109,15 @@
     auto key = conf.key;
     player->on_client_disconnected().connect([this, key]()
     {
-        remove_player_for_key(key);
+        // Call remove_player_for_key asynchronously otherwise deadlock can occur
+        // if called within this dispatcher context.
+        // remove_player_for_key can destroy the player instance which in turn
+        // destroys the "on_client_disconnected" signal whose destructor will wait
+        // until all dispatches are done
+        d->io_service.post([this, key]()
+        {
+            remove_player_for_key(key);
+        });
     });
 
     return player;