unity-scopes-api

Merge lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages into lp:unity-scopes-api/devel

  1. debug_dbus_messages
  2. Merge into devel
Proposed by Marcus Tomlinson
Status: Merged
Approved by: Paweł Stołowski
Approved revision: 474
Merged at revision: 468
Proposed branch: lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages
Merge into: lp:unity-scopes-api/devel
Diff against target: 215 lines (+67/-16)
7 files modified
debian/libunity-scopes3.symbols (+1/-0)
include/unity/scopes/internal/RegistryObject.h (+2/-0)
include/unity/scopes/internal/Utils.h (+2/-0)
scoperegistry/scoperegistry.cpp (+1/-0)
src/scopes/internal/RegistryObject.cpp (+51/-15)
src/scopes/internal/Utils.cpp (+8/-0)
src/scopes/internal/smartscopes/SSRegistryObject.cpp (+2/-1)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages
Reviewer Review Type Date Requested Status
Paweł Stołowski (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Michi Henning (community) Approve
Review via email: mp+232068@code.launchpad.net

Commit message

Execute SDK DBus callback on scope start and stop in debug mode.

To post a comment you must log in.
lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages updated
469. By Marcus Tomlinson

Whoops, reverted intentional break

470. By Marcus Tomlinson

Fixed formatting

Revision history for this message
Paweł Stołowski (stolowski) wrote :

91 + std::string started_message = "dbus-send --type=method_call --dest=com.ubuntu.SDKAppLaunch "
92 + "/ScopeRegistryCallback com.ubuntu.SDKAppLaunch.ScopeLoaded "
93 + "string:" + exec_data_.scope_id + " "
94 + "uint64:" + std::to_string(process_.pid());
95 + if (std::system(started_message.c_str()) != 0)

I think dbus-send by default would block for up to 25 seconds waiting for reply if there is a problemwith receiver; is registry going to block for that long then?

review: Needs Information
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> 91 + std::string started_message = "dbus-send --type=method_call
> --dest=com.ubuntu.SDKAppLaunch "
> 92 + "/ScopeRegistryCallback com.ubuntu.SDKAppLaunch.ScopeLoaded "
> 93 + "string:" + exec_data_.scope_id + " "
> 94 + "uint64:" + std::to_string(process_.pid());
> 95 + if (std::system(started_message.c_str()) != 0)
>
> I think dbus-send by default would block for up to 25 seconds waiting for
> reply if there is a problemwith receiver; is registry going to block for that
> long then?

The remote methods being called are void methods, no return values. I've tried dbus-send without the service running and it returns immediately. I then put a 20s sleep in the callback method on the python side, and the dbus-send still returns immediately. So looks like we're fine

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:470
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1127/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/685/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/680/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/679/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1127/rebuild

review: Needs Fixing (continuous-integration)
lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages updated
471. By Marcus Tomlinson

Updated symbols

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:471
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1128/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/686
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/681
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/681/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/680

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1128/rebuild

review: Approve (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Looks good, I've just one request: could you move the arguments of dbus-send calls into const strings somewhere, preferably at the top of the source file?

review: Needs Fixing
Revision history for this message
Michi Henning (michihenning) wrote :

93 + if (scope_started)
...
113 + else if (scope_started == false)

A simple "else" would do.

28 + void publish_state_change(bool scope_started);

Maybe use an enum Started, Stopped here? This would make the code at the call site much easier to read:

    publish_state_change(Started)

instead of

    publish_state_change(true);

if (std::system(started_message.c_str()) != 0)

This looks like a crash or a hang waiting to happen.

system() is not thread-safe. If we ever end up here concurrently (which is possible, I believe), we'll have a problem. I think it's necessary to serialise all calls to system.

review: Needs Fixing
Revision history for this message
Michi Henning (michihenning) wrote :

109 + std::cerr << "RegistryObject::ScopeProcess::publish_state_change(): Failed to execute SDK DBus callback" << endl;

It would be nice to have at least the scope ID in the error message (same with the exception a few lines below).

review: Needs Fixing
Revision history for this message
Michi Henning (michihenning) wrote :

I just noticed that there is also a call to system() in SSRegistryObject. I don't think we ever have both RegistryObject and SSRegistryObject instantiated in the same process, so we can't race there. But this suggests that we probably should have a utility class that wraps calls to system so they are serialised.

lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages updated
472. By Marcus Tomlinson

Addressed review comments

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> Looks good, I've just one request: could you move the arguments of dbus-send
> calls into const strings somewhere, preferably at the top of the source file?

done

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> 93 + if (scope_started)
> ...
> 113 + else if (scope_started == false)
>
> A simple "else" would do.
>
> 28 + void publish_state_change(bool scope_started);
>
> Maybe use an enum Started, Stopped here? This would make the code at the call
> site much easier to read:
>
> publish_state_change(Started)
>
> instead of
>
> publish_state_change(true);

done

>
> if (std::system(started_message.c_str()) != 0)
>
> This looks like a crash or a hang waiting to happen.
>
> system() is not thread-safe. If we ever end up here concurrently (which is
> possible, I believe), we'll have a problem. I think it's necessary to
> serialise all calls to system.

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> 109 + std::cerr <<
> "RegistryObject::ScopeProcess::publish_state_change(): Failed to execute SDK
> DBus callback" << endl;
>
> It would be nice to have at least the scope ID in the error message (same with
> the exception a few lines below).

done

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> I just noticed that there is also a call to system() in SSRegistryObject. I
> don't think we ever have both RegistryObject and SSRegistryObject instantiated
> in the same process, so we can't race there. But this suggests that we
> probably should have a utility class that wraps calls to system so they are
> serialised.

done

lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages updated
473. By Marcus Tomlinson

Move mutex into safe_system_call

Revision history for this message
Michi Henning (michihenning) wrote :

Looks good to me now, thanks!

review: Approve
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> Looks good to me now, thanks!

Thank you for the thorough review :)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:472
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1129/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/687/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/682/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/681/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1129/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:473
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1130/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/688/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/683/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/682/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1130/rebuild

review: Needs Fixing (continuous-integration)
lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages updated
474. By Marcus Tomlinson

Updated symbols

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:474
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1131/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/689
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/684
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/684/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/683

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1131/rebuild

review: Approve (continuous-integration)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

Thanks, LGTM!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'debian/libunity-scopes3.symbols'
--- debian/libunity-scopes3.symbols 2014-08-19 08:25:14 +0000
+++ debian/libunity-scopes3.symbols 2014-08-26 05:49:51 +0000
@@ -507,6 +507,7 @@
507 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::update_state(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0.4.2+14.04.20140404.2507 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::update_state(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0.4.2+14.04.20140404.2
508 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::on_process_death(int)@Base" 0.4.2+14.04.20140404.2508 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::on_process_death(int)@Base" 0.4.2+14.04.20140404.2
509 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::expand_custom_exec()@Base" 0.5.0+14.10.20140619509 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::expand_custom_exec()@Base" 0.5.0+14.10.20140619
510 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::publish_state_change(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0replaceme
510 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::clear_handle_unlocked()@Base" 0.4.2+14.04.20140404.2511 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::clear_handle_unlocked()@Base" 0.4.2+14.04.20140404.2
511 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::update_state_unlocked(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0.4.2+14.04.20140404.2512 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::update_state_unlocked(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0.4.2+14.04.20140404.2
512 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::exec(core::posix::ChildProcess::DeathObserver&, std::shared_ptr<unity::scopes::internal::Executor>)@Base" 0.4.3+14.10.20140428513 (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::exec(core::posix::ChildProcess::DeathObserver&, std::shared_ptr<unity::scopes::internal::Executor>)@Base" 0.4.3+14.10.20140428
513514
=== modified file 'include/unity/scopes/internal/RegistryObject.h'
--- include/unity/scopes/internal/RegistryObject.h 2014-08-08 06:44:28 +0000
+++ include/unity/scopes/internal/RegistryObject.h 2014-08-26 05:49:51 +0000
@@ -53,6 +53,7 @@
53 std::string scope_config;53 std::string scope_config;
54 std::string confinement_profile;54 std::string confinement_profile;
55 int timeout_ms;55 int timeout_ms;
56 bool debug_mode;
56 };57 };
5758
58public:59public:
@@ -111,6 +112,7 @@
111 void kill(std::unique_lock<std::mutex>& lock);112 void kill(std::unique_lock<std::mutex>& lock);
112113
113 std::vector<std::string> expand_custom_exec();114 std::vector<std::string> expand_custom_exec();
115 void publish_state_change(ProcessState scope_state);
114116
115 private:117 private:
116 const ScopeExecData exec_data_;118 const ScopeExecData exec_data_;
117119
=== modified file 'include/unity/scopes/internal/Utils.h'
--- include/unity/scopes/internal/Utils.h 2014-06-25 16:29:26 +0000
+++ include/unity/scopes/internal/Utils.h 2014-08-26 05:49:51 +0000
@@ -54,6 +54,8 @@
54template<>54template<>
55bool convert_to<bool>(std::string const& val, Variant& out);55bool convert_to<bool>(std::string const& val, Variant& out);
5656
57int safe_system_call(std::string const& command);
58
57} // namespace internal59} // namespace internal
5860
59} // namespace scopes61} // namespace scopes
6062
=== modified file 'scoperegistry/scoperegistry.cpp'
--- scoperegistry/scoperegistry.cpp 2014-08-17 01:47:41 +0000
+++ scoperegistry/scoperegistry.cpp 2014-08-26 05:49:51 +0000
@@ -428,6 +428,7 @@
428 }428 }
429 exec_data.runtime_config = config_file;429 exec_data.runtime_config = config_file;
430 exec_data.scope_config = scope.second;430 exec_data.scope_config = scope.second;
431 exec_data.debug_mode = sc.debug_mode();
431432
432 registry->add_local_scope(scope.first, move(meta), exec_data);433 registry->add_local_scope(scope.first, move(meta), exec_data);
433}434}
434435
=== modified file 'src/scopes/internal/RegistryObject.cpp'
--- src/scopes/internal/RegistryObject.cpp 2014-08-18 06:07:34 +0000
+++ src/scopes/internal/RegistryObject.cpp 2014-08-26 05:49:51 +0000
@@ -20,6 +20,7 @@
2020
21#include <unity/scopes/internal/MWRegistry.h>21#include <unity/scopes/internal/MWRegistry.h>
22#include <unity/scopes/internal/RuntimeImpl.h>22#include <unity/scopes/internal/RuntimeImpl.h>
23#include <unity/scopes/internal/Utils.h>
23#include <unity/scopes/ScopeExceptions.h>24#include <unity/scopes/ScopeExceptions.h>
24#include <unity/UnityExceptions.h>25#include <unity/UnityExceptions.h>
25#include <unity/util/ResourcePtr.h>26#include <unity/util/ResourcePtr.h>
@@ -35,6 +36,9 @@
3536
36using namespace std;37using namespace std;
3738
39static const char* c_debug_dbus_started_cmd = "dbus-send --type=method_call --dest=com.ubuntu.SDKAppLaunch /ScopeRegistryCallback com.ubuntu.SDKAppLaunch.ScopeLoaded";
40static const char* c_debug_dbus_stopped_cmd = "dbus-send --type=method_call --dest=com.ubuntu.SDKAppLaunch /ScopeRegistryCallback com.ubuntu.SDKAppLaunch.ScopeStopped";
41
38namespace unity42namespace unity
39{43{
4044
@@ -524,11 +528,7 @@
524 }528 }
525 else if (new_state == Running)529 else if (new_state == Running)
526 {530 {
527 if (reg_publisher_)531 publish_state_change(Running);
528 {
529 // Send a "started" message to subscribers to inform them that this scope (topic) has started
530 reg_publisher_->send_message("started", exec_data_.scope_id);
531 }
532532
533 if (state_ != Starting)533 if (state_ != Starting)
534 {534 {
@@ -540,11 +540,7 @@
540 }540 }
541 else if (new_state == Stopped)541 else if (new_state == Stopped)
542 {542 {
543 if (reg_publisher_)543 publish_state_change(Stopped);
544 {
545 // Send a "stopped" message to subscribers to inform them that this scope (topic) has stopped
546 reg_publisher_->send_message("stopped", exec_data_.scope_id);
547 }
548544
549 if (state_ != Stopping)545 if (state_ != Stopping)
550 {546 {
@@ -554,11 +550,7 @@
554 }550 }
555 else if (new_state == Stopping && manually_started_)551 else if (new_state == Stopping && manually_started_)
556 {552 {
557 if (reg_publisher_)553 publish_state_change(Stopped);
558 {
559 // Send a "stopped" message to subscribers to inform them that this scope (topic) has stopped
560 reg_publisher_->send_message("stopped", exec_data_.scope_id);
561 }
562554
563 cout << "RegistryObject::ScopeProcess: Manually started process for scope: \""555 cout << "RegistryObject::ScopeProcess: Manually started process for scope: \""
564 << exec_data_.scope_id << "\" exited" << endl;556 << exec_data_.scope_id << "\" exited" << endl;
@@ -664,6 +656,50 @@
664 return command_args;656 return command_args;
665}657}
666658
659void RegistryObject::ScopeProcess::publish_state_change(ProcessState scope_state)
660{
661 if (scope_state == Running)
662 {
663 if (reg_publisher_)
664 {
665 // Send a "started" message to subscribers to inform them that this scope (topic) has started
666 reg_publisher_->send_message("started", exec_data_.scope_id);
667 }
668 if (exec_data_.debug_mode)
669 {
670 // If we're in debug mode, callback to the SDK via dbus (used to monitor scope lifecycle)
671 std::string started_message = c_debug_dbus_started_cmd;
672 started_message += " string:" + exec_data_.scope_id + " uint64:" + std::to_string(process_.pid());
673 if (safe_system_call(started_message) != 0)
674 {
675 std::cerr << "RegistryObject::ScopeProcess::publish_state_change(): "
676 "Failed to execute SDK DBus ScopeLoaded callback "
677 "(Scope ID: " << exec_data_.scope_id << ")" << endl;
678 }
679 }
680 }
681 else if (scope_state == Stopped)
682 {
683 if (reg_publisher_)
684 {
685 // Send a "stopped" message to subscribers to inform them that this scope (topic) has stopped
686 reg_publisher_->send_message("stopped", exec_data_.scope_id);
687 }
688 if (exec_data_.debug_mode)
689 {
690 // If we're in debug mode, callback to the SDK via dbus (used to monitor scope lifecycle)
691 std::string stopped_message = c_debug_dbus_stopped_cmd;
692 stopped_message += " string:" + exec_data_.scope_id;
693 if (safe_system_call(stopped_message) != 0)
694 {
695 std::cerr << "RegistryObject::ScopeProcess::publish_state_change(): "
696 "Failed to execute SDK DBus ScopeStopped callback "
697 "(Scope ID: " << exec_data_.scope_id << ")" << endl;
698 }
699 }
700 }
701}
702
667} // namespace internal703} // namespace internal
668704
669} // namespace scopes705} // namespace scopes
670706
=== modified file 'src/scopes/internal/Utils.cpp'
--- src/scopes/internal/Utils.cpp 2014-08-05 05:29:56 +0000
+++ src/scopes/internal/Utils.cpp 2014-08-26 05:49:51 +0000
@@ -21,6 +21,7 @@
21#include <unity/UnityExceptions.h>21#include <unity/UnityExceptions.h>
22#include <iomanip>22#include <iomanip>
23#include <locale>23#include <locale>
24#include <mutex>
2425
25namespace unity26namespace unity
26{27{
@@ -142,6 +143,13 @@
142 return false;143 return false;
143}144}
144145
146int safe_system_call(std::string const& command)
147{
148 static std::mutex system_mutex;
149 std::lock_guard<std::mutex> lock(system_mutex);
150 return std::system(command.c_str());
151}
152
145} // namespace internal153} // namespace internal
146154
147} // namespace scopes155} // namespace scopes
148156
=== modified file 'src/scopes/internal/smartscopes/SSRegistryObject.cpp'
--- src/scopes/internal/smartscopes/SSRegistryObject.cpp 2014-08-20 07:58:15 +0000
+++ src/scopes/internal/smartscopes/SSRegistryObject.cpp 2014-08-26 05:49:51 +0000
@@ -25,6 +25,7 @@
25#include <unity/scopes/internal/ScopeImpl.h>25#include <unity/scopes/internal/ScopeImpl.h>
26#include <unity/scopes/internal/ScopeMetadataImpl.h>26#include <unity/scopes/internal/ScopeMetadataImpl.h>
27#include <unity/scopes/internal/smartscopes/HttpClientQt.h>27#include <unity/scopes/internal/smartscopes/HttpClientQt.h>
28#include <unity/scopes/internal/Utils.h>
28#include <unity/scopes/ScopeExceptions.h>29#include <unity/scopes/ScopeExceptions.h>
29#include <unity/UnityExceptions.h>30#include <unity/UnityExceptions.h>
3031
@@ -374,7 +375,7 @@
374 if (changed)375 if (changed)
375 {376 {
376 // something has changed, send invalidate signal377 // something has changed, send invalidate signal
377 int result = system(c_dbussend_cmd);378 int result = safe_system_call(c_dbussend_cmd);
378 (void)result;379 (void)result;
379 }380 }
380}381}

Subscribers

People subscribed via source and target branches

to all changes: