Merge lp:~marcustomlinson/unity-scopes-api/debug_dbus_messages into lp:unity-scopes-api/devel
- debug_dbus_messages
- Merge into devel
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 |
Related bugs: |
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.
Description of the change
- 469. By Marcus Tomlinson
-
Whoops, reverted intentional break
- 470. By Marcus Tomlinson
-
Fixed formatting
Marcus Tomlinson (marcustomlinson) wrote : | # |
> 91 + std::string started_message = "dbus-send --type=method_call
> --dest=
> 92 + "/ScopeRegistry
> 93 + "string:" + exec_data_.scope_id + " "
> 94 + "uint64:" + std::to_
> 95 + if (std::system(
>
> 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
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:470
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 471. By Marcus Tomlinson
-
Updated symbols
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:471
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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?
Michi Henning (michihenning) wrote : | # |
93 + if (scope_started)
...
113 + else if (scope_started == false)
A simple "else" would do.
28 + void publish_
Maybe use an enum Started, Stopped here? This would make the code at the call site much easier to read:
publish_
instead of
publish_
if (std::system(
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.
Michi Henning (michihenning) wrote : | # |
109 + std::cerr << "RegistryObject
It would be nice to have at least the scope ID in the error message (same with the exception a few lines below).
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.
- 472. By Marcus Tomlinson
-
Addressed review comments
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
Marcus Tomlinson (marcustomlinson) wrote : | # |
> 93 + if (scope_started)
> ...
> 113 + else if (scope_started == false)
>
> A simple "else" would do.
>
> 28 + void publish_
>
> Maybe use an enum Started, Stopped here? This would make the code at the call
> site much easier to read:
>
> publish_
>
> instead of
>
> publish_
done
>
> if (std::system(
>
> 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.
Marcus Tomlinson (marcustomlinson) wrote : | # |
> 109 + std::cerr <<
> "RegistryObject
> 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
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
- 473. By Marcus Tomlinson
-
Move mutex into safe_system_call
Michi Henning (michihenning) wrote : | # |
Looks good to me now, thanks!
Marcus Tomlinson (marcustomlinson) wrote : | # |
> Looks good to me now, thanks!
Thank you for the thorough review :)
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:472
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:473
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 474. By Marcus Tomlinson
-
Updated symbols
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:474
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Paweł Stołowski (stolowski) wrote : | # |
Thanks, LGTM!
Preview Diff
1 | === modified file 'debian/libunity-scopes3.symbols' |
2 | --- debian/libunity-scopes3.symbols 2014-08-19 08:25:14 +0000 |
3 | +++ debian/libunity-scopes3.symbols 2014-08-26 05:49:51 +0000 |
4 | @@ -507,6 +507,7 @@ |
5 | (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::update_state(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0.4.2+14.04.20140404.2 |
6 | (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::on_process_death(int)@Base" 0.4.2+14.04.20140404.2 |
7 | (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::expand_custom_exec()@Base" 0.5.0+14.10.20140619 |
8 | + (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::publish_state_change(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0replaceme |
9 | (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::clear_handle_unlocked()@Base" 0.4.2+14.04.20140404.2 |
10 | (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::update_state_unlocked(unity::scopes::internal::RegistryObject::ScopeProcess::ProcessState)@Base" 0.4.2+14.04.20140404.2 |
11 | (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 |
12 | |
13 | === modified file 'include/unity/scopes/internal/RegistryObject.h' |
14 | --- include/unity/scopes/internal/RegistryObject.h 2014-08-08 06:44:28 +0000 |
15 | +++ include/unity/scopes/internal/RegistryObject.h 2014-08-26 05:49:51 +0000 |
16 | @@ -53,6 +53,7 @@ |
17 | std::string scope_config; |
18 | std::string confinement_profile; |
19 | int timeout_ms; |
20 | + bool debug_mode; |
21 | }; |
22 | |
23 | public: |
24 | @@ -111,6 +112,7 @@ |
25 | void kill(std::unique_lock<std::mutex>& lock); |
26 | |
27 | std::vector<std::string> expand_custom_exec(); |
28 | + void publish_state_change(ProcessState scope_state); |
29 | |
30 | private: |
31 | const ScopeExecData exec_data_; |
32 | |
33 | === modified file 'include/unity/scopes/internal/Utils.h' |
34 | --- include/unity/scopes/internal/Utils.h 2014-06-25 16:29:26 +0000 |
35 | +++ include/unity/scopes/internal/Utils.h 2014-08-26 05:49:51 +0000 |
36 | @@ -54,6 +54,8 @@ |
37 | template<> |
38 | bool convert_to<bool>(std::string const& val, Variant& out); |
39 | |
40 | +int safe_system_call(std::string const& command); |
41 | + |
42 | } // namespace internal |
43 | |
44 | } // namespace scopes |
45 | |
46 | === modified file 'scoperegistry/scoperegistry.cpp' |
47 | --- scoperegistry/scoperegistry.cpp 2014-08-17 01:47:41 +0000 |
48 | +++ scoperegistry/scoperegistry.cpp 2014-08-26 05:49:51 +0000 |
49 | @@ -428,6 +428,7 @@ |
50 | } |
51 | exec_data.runtime_config = config_file; |
52 | exec_data.scope_config = scope.second; |
53 | + exec_data.debug_mode = sc.debug_mode(); |
54 | |
55 | registry->add_local_scope(scope.first, move(meta), exec_data); |
56 | } |
57 | |
58 | === modified file 'src/scopes/internal/RegistryObject.cpp' |
59 | --- src/scopes/internal/RegistryObject.cpp 2014-08-18 06:07:34 +0000 |
60 | +++ src/scopes/internal/RegistryObject.cpp 2014-08-26 05:49:51 +0000 |
61 | @@ -20,6 +20,7 @@ |
62 | |
63 | #include <unity/scopes/internal/MWRegistry.h> |
64 | #include <unity/scopes/internal/RuntimeImpl.h> |
65 | +#include <unity/scopes/internal/Utils.h> |
66 | #include <unity/scopes/ScopeExceptions.h> |
67 | #include <unity/UnityExceptions.h> |
68 | #include <unity/util/ResourcePtr.h> |
69 | @@ -35,6 +36,9 @@ |
70 | |
71 | using namespace std; |
72 | |
73 | +static const char* c_debug_dbus_started_cmd = "dbus-send --type=method_call --dest=com.ubuntu.SDKAppLaunch /ScopeRegistryCallback com.ubuntu.SDKAppLaunch.ScopeLoaded"; |
74 | +static const char* c_debug_dbus_stopped_cmd = "dbus-send --type=method_call --dest=com.ubuntu.SDKAppLaunch /ScopeRegistryCallback com.ubuntu.SDKAppLaunch.ScopeStopped"; |
75 | + |
76 | namespace unity |
77 | { |
78 | |
79 | @@ -524,11 +528,7 @@ |
80 | } |
81 | else if (new_state == Running) |
82 | { |
83 | - if (reg_publisher_) |
84 | - { |
85 | - // Send a "started" message to subscribers to inform them that this scope (topic) has started |
86 | - reg_publisher_->send_message("started", exec_data_.scope_id); |
87 | - } |
88 | + publish_state_change(Running); |
89 | |
90 | if (state_ != Starting) |
91 | { |
92 | @@ -540,11 +540,7 @@ |
93 | } |
94 | else if (new_state == Stopped) |
95 | { |
96 | - if (reg_publisher_) |
97 | - { |
98 | - // Send a "stopped" message to subscribers to inform them that this scope (topic) has stopped |
99 | - reg_publisher_->send_message("stopped", exec_data_.scope_id); |
100 | - } |
101 | + publish_state_change(Stopped); |
102 | |
103 | if (state_ != Stopping) |
104 | { |
105 | @@ -554,11 +550,7 @@ |
106 | } |
107 | else if (new_state == Stopping && manually_started_) |
108 | { |
109 | - if (reg_publisher_) |
110 | - { |
111 | - // Send a "stopped" message to subscribers to inform them that this scope (topic) has stopped |
112 | - reg_publisher_->send_message("stopped", exec_data_.scope_id); |
113 | - } |
114 | + publish_state_change(Stopped); |
115 | |
116 | cout << "RegistryObject::ScopeProcess: Manually started process for scope: \"" |
117 | << exec_data_.scope_id << "\" exited" << endl; |
118 | @@ -664,6 +656,50 @@ |
119 | return command_args; |
120 | } |
121 | |
122 | +void RegistryObject::ScopeProcess::publish_state_change(ProcessState scope_state) |
123 | +{ |
124 | + if (scope_state == Running) |
125 | + { |
126 | + if (reg_publisher_) |
127 | + { |
128 | + // Send a "started" message to subscribers to inform them that this scope (topic) has started |
129 | + reg_publisher_->send_message("started", exec_data_.scope_id); |
130 | + } |
131 | + if (exec_data_.debug_mode) |
132 | + { |
133 | + // If we're in debug mode, callback to the SDK via dbus (used to monitor scope lifecycle) |
134 | + std::string started_message = c_debug_dbus_started_cmd; |
135 | + started_message += " string:" + exec_data_.scope_id + " uint64:" + std::to_string(process_.pid()); |
136 | + if (safe_system_call(started_message) != 0) |
137 | + { |
138 | + std::cerr << "RegistryObject::ScopeProcess::publish_state_change(): " |
139 | + "Failed to execute SDK DBus ScopeLoaded callback " |
140 | + "(Scope ID: " << exec_data_.scope_id << ")" << endl; |
141 | + } |
142 | + } |
143 | + } |
144 | + else if (scope_state == Stopped) |
145 | + { |
146 | + if (reg_publisher_) |
147 | + { |
148 | + // Send a "stopped" message to subscribers to inform them that this scope (topic) has stopped |
149 | + reg_publisher_->send_message("stopped", exec_data_.scope_id); |
150 | + } |
151 | + if (exec_data_.debug_mode) |
152 | + { |
153 | + // If we're in debug mode, callback to the SDK via dbus (used to monitor scope lifecycle) |
154 | + std::string stopped_message = c_debug_dbus_stopped_cmd; |
155 | + stopped_message += " string:" + exec_data_.scope_id; |
156 | + if (safe_system_call(stopped_message) != 0) |
157 | + { |
158 | + std::cerr << "RegistryObject::ScopeProcess::publish_state_change(): " |
159 | + "Failed to execute SDK DBus ScopeStopped callback " |
160 | + "(Scope ID: " << exec_data_.scope_id << ")" << endl; |
161 | + } |
162 | + } |
163 | + } |
164 | +} |
165 | + |
166 | } // namespace internal |
167 | |
168 | } // namespace scopes |
169 | |
170 | === modified file 'src/scopes/internal/Utils.cpp' |
171 | --- src/scopes/internal/Utils.cpp 2014-08-05 05:29:56 +0000 |
172 | +++ src/scopes/internal/Utils.cpp 2014-08-26 05:49:51 +0000 |
173 | @@ -21,6 +21,7 @@ |
174 | #include <unity/UnityExceptions.h> |
175 | #include <iomanip> |
176 | #include <locale> |
177 | +#include <mutex> |
178 | |
179 | namespace unity |
180 | { |
181 | @@ -142,6 +143,13 @@ |
182 | return false; |
183 | } |
184 | |
185 | +int safe_system_call(std::string const& command) |
186 | +{ |
187 | + static std::mutex system_mutex; |
188 | + std::lock_guard<std::mutex> lock(system_mutex); |
189 | + return std::system(command.c_str()); |
190 | +} |
191 | + |
192 | } // namespace internal |
193 | |
194 | } // namespace scopes |
195 | |
196 | === modified file 'src/scopes/internal/smartscopes/SSRegistryObject.cpp' |
197 | --- src/scopes/internal/smartscopes/SSRegistryObject.cpp 2014-08-20 07:58:15 +0000 |
198 | +++ src/scopes/internal/smartscopes/SSRegistryObject.cpp 2014-08-26 05:49:51 +0000 |
199 | @@ -25,6 +25,7 @@ |
200 | #include <unity/scopes/internal/ScopeImpl.h> |
201 | #include <unity/scopes/internal/ScopeMetadataImpl.h> |
202 | #include <unity/scopes/internal/smartscopes/HttpClientQt.h> |
203 | +#include <unity/scopes/internal/Utils.h> |
204 | #include <unity/scopes/ScopeExceptions.h> |
205 | #include <unity/UnityExceptions.h> |
206 | |
207 | @@ -374,7 +375,7 @@ |
208 | if (changed) |
209 | { |
210 | // something has changed, send invalidate signal |
211 | - int result = system(c_dbussend_cmd); |
212 | + int result = safe_system_call(c_dbussend_cmd); |
213 | (void)result; |
214 | } |
215 | } |
91 + std::string started_message = "dbus-send --type=method_call --dest= com.ubuntu. SDKAppLaunch " Callback com.ubuntu. SDKAppLaunch. ScopeLoaded " string( process_ .pid()) ; started_ message. c_str() ) != 0)
92 + "/ScopeRegistry
93 + "string:" + exec_data_.scope_id + " "
94 + "uint64:" + std::to_
95 + if (std::system(
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?