unity-scopes-api

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

  1. fix_oa_slow_start
  2. Merge into devel
Proposed by Marcus Tomlinson
Status: Merged
Approved by: Marcus Tomlinson
Approved revision: 272
Merged at revision: 506
Proposed branch: lp:~marcustomlinson/unity-scopes-api/fix_oa_slow_start
Merge into: lp:unity-scopes-api/devel
Diff against target: 270 lines (+54/-40)
4 files modified
include/unity/scopes/OnlineAccountClient.h (+1/-1)
include/unity/scopes/internal/OnlineAccountClientImpl.h (+2/-1)
src/scopes/internal/OnlineAccountClientImpl.cpp (+41/-28)
test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp (+10/-10)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/fix_oa_slow_start
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Paweł Stołowski (community) Approve
Review via email: mp+237932@code.launchpad.net

Commit message

Move flush_pending_session() out of refresh_service_statuses() and into get_service_statuses() to defer the wait for SignOn replies until statuses are pulled. Also, run the callback in a seperate thread so that get_service_statuses() can be called from it.

To post a comment you must log in.
Revision history for this message
Paweł Stołowski (stolowski) wrote :

+1

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

FAILED: Continuous integration, rev:269
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1215/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/773/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/768
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/768/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/767

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

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

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-autolanding/534/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-autolanding/358/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-autolanding/359
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-autolanding/359/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-autolanding/358

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

Allow busy signon sessions to fully complete before shutting down (only cancel overlapping sessions), and clear the session from within callback() after the callback has been invoked.

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

FAILED: Continuous integration, rev:270
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1216/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/774
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/769
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/769/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/768/console

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

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

Whoops, initialize got_update_ to false

272. By Marcus Tomlinson

use uint for account_id instead of int

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

PASSED: Continuous integration, rev:271
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1217/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-amd64-ci/775
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/770
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-armhf-ci/770/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-utopic-i386-ci/769

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/unity/scopes/OnlineAccountClient.h'
2--- include/unity/scopes/OnlineAccountClient.h 2014-10-09 16:13:40 +0000
3+++ include/unity/scopes/OnlineAccountClient.h 2014-10-10 13:41:13 +0000
4@@ -60,7 +60,7 @@
5 */
6 struct ServiceStatus
7 {
8- int account_id; ///< A unique ID of the online account parenting this service.
9+ uint account_id; ///< A unique ID of the online account parenting this service.
10 bool service_enabled; ///< True if this service is enabled.
11 bool service_authenticated; ///< True if this service is authenticated.
12 std::string client_id; ///< "ConsumerKey" / "ClientId" OAuth (1 / 2) parameter.
13
14=== modified file 'include/unity/scopes/internal/OnlineAccountClientImpl.h'
15--- include/unity/scopes/internal/OnlineAccountClientImpl.h 2014-10-09 16:13:40 +0000
16+++ include/unity/scopes/internal/OnlineAccountClientImpl.h 2014-10-10 13:41:13 +0000
17@@ -89,7 +89,7 @@
18 std::string service_name();
19 std::shared_ptr<GMainContext> main_loop_context();
20
21- void callback(AccountInfo const* info, std::string const& error = "");
22+ void callback(AccountInfo* info, std::string const& error = "");
23
24 bool has_account(AgAccountId const& account_id);
25 void add_account(AgAccountId const& account_id, std::shared_ptr<AccountInfo> account_info);
26@@ -103,6 +103,7 @@
27
28 std::mutex callback_mutex_;
29 OnlineAccountClient::ServiceUpdateCallback callback_;
30+ std::thread callback_thread_;
31
32 std::mutex mutex_;
33 std::condition_variable cond_;
34
35=== modified file 'src/scopes/internal/OnlineAccountClientImpl.cpp'
36--- src/scopes/internal/OnlineAccountClientImpl.cpp 2014-10-09 16:13:40 +0000
37+++ src/scopes/internal/OnlineAccountClientImpl.cpp 2014-10-10 13:41:13 +0000
38@@ -89,16 +89,6 @@
39 return service_status;
40 }
41
42-static void clear_session(AccountInfo* info)
43-{
44- if (info->session)
45- {
46- // Cancel the session in case its still busy
47- signon_auth_session_cancel(info->session.get());
48- }
49- info->session = nullptr;
50-}
51-
52 static gboolean main_loop_is_running_cb(void* user_data)
53 {
54 OnlineAccountClientImpl* account_client = reinterpret_cast<OnlineAccountClientImpl*>(user_data);
55@@ -131,16 +121,18 @@
56 std::shared_ptr<GError> error_cleanup(error, free_error);
57
58 info->account_client->callback(info, error ? error->message : "");
59-
60- // Clear session info
61- clear_session(info);
62 }
63
64 static void service_update_cb(AgAccountService* account_service, gboolean enabled, AccountInfo* info)
65 {
66+ // If another session is currently busy, cancel it
67+ if (info->session)
68+ {
69+ signon_auth_session_cancel(info->session.get());
70+ }
71+
72 // Service state has updated, clear the old session data
73 info->service_enabled = enabled;
74- clear_session(info);
75
76 if (enabled)
77 {
78@@ -252,10 +244,10 @@
79 {
80 // Quit the main loop, causing the thread to exit
81 g_main_loop_quit(main_loop_.get());
82- if (main_loop_thread_.joinable())
83- {
84- main_loop_thread_.join();
85- }
86+ }
87+ if (main_loop_thread_.joinable())
88+ {
89+ main_loop_thread_.join();
90 }
91 if (thread_exception_)
92 {
93@@ -318,6 +310,14 @@
94 {
95 tear_down();
96 }
97+
98+ std::unique_lock<std::mutex> lock(callback_mutex_);
99+ if (callback_thread_.joinable())
100+ {
101+ lock.unlock();
102+ callback_thread_.join();
103+ lock.lock();
104+ }
105 }
106
107 void OnlineAccountClientImpl::set_service_update_callback(OnlineAccountClient::ServiceUpdateCallback callback)
108@@ -353,13 +353,12 @@
109 account_enabled_cb(manager_.get(), account_id, this);
110 lock.lock();
111 }
112- flush_pending_session(account_id, lock);
113 }
114 }
115
116 std::vector<OnlineAccountClient::ServiceStatus> OnlineAccountClientImpl::get_service_statuses()
117 {
118- std::lock_guard<std::mutex> lock(mutex_);
119+ std::unique_lock<std::mutex> lock(mutex_);
120 if (thread_exception_)
121 {
122 std::rethrow_exception(thread_exception_); // LCOV_EXCL_LINE
123@@ -369,6 +368,7 @@
124 std::vector<OnlineAccountClient::ServiceStatus> service_statuses;
125 for (auto const& info : accounts_)
126 {
127+ flush_pending_session(info.second->account_id, lock);
128 service_statuses.push_back(info_to_details(info.second.get()));
129 }
130 return service_statuses;
131@@ -435,7 +435,6 @@
132 {
133 // Before we nuke the map, ensure that any pending sessions are done
134 g_signal_handler_disconnect(info.second->account_service.get(), info.second->service_update_signal_id_);
135- clear_session(info.second.get());
136 flush_pending_session(info.second->account_id, lock);
137 }
138 accounts_.clear();
139@@ -455,7 +454,7 @@
140 // Wait until all currently running login sessions are done
141 // (ensures that accounts_ is up to date)
142 std::shared_ptr<GMainLoop> event_loop;
143- event_loop.reset(g_main_loop_new(g_main_context_get_thread_default(), true), g_main_loop_unref);
144+ event_loop.reset(g_main_loop_new(main_loop_context_.get(), true), g_main_loop_unref);
145 while(info->session)
146 {
147 // We need to wait inside an event loop to allow for the main application loop to
148@@ -463,7 +462,7 @@
149 std::shared_ptr<GSource> source;
150 source.reset(g_timeout_source_new(10), g_source_unref);
151 g_source_set_callback(source.get(), wake_up_event_loop_cb, event_loop.get(), NULL);
152- g_source_attach(source.get(), g_main_context_get_thread_default());
153+ g_source_attach(source.get(), main_loop_context_.get());
154
155 lock.unlock();
156 g_main_loop_run(event_loop.get());
157@@ -508,13 +507,28 @@
158 return main_loop_context_;
159 }
160
161-void OnlineAccountClientImpl::callback(AccountInfo const* info, std::string const& error)
162+void OnlineAccountClientImpl::callback(AccountInfo* info, std::string const& error)
163 {
164- std::lock_guard<std::mutex> lock(callback_mutex_);
165- if (callback_)
166+ std::unique_lock<std::mutex> lock(callback_mutex_);
167+ if (callback_thread_.joinable())
168 {
169- callback_(info_to_details(info, error));
170+ lock.unlock();
171+ callback_thread_.join();
172+ lock.lock();
173 }
174+
175+ OnlineAccountClient::ServiceStatus status = info_to_details(info, error);
176+ callback_thread_ = std::thread([this, status]
177+ {
178+ std::lock_guard<std::mutex> lock(callback_mutex_);
179+ if (callback_)
180+ {
181+ callback_(status);
182+ }
183+ });
184+
185+ // Clear session info
186+ info->session = nullptr;
187 }
188
189 bool OnlineAccountClientImpl::has_account(AgAccountId const& account_id)
190@@ -543,7 +557,6 @@
191
192 // Before we nuke the pointer, ensure that any pending sessions are done
193 g_signal_handler_disconnect(info->account_service.get(), info->service_update_signal_id_);
194- clear_session(info.get());
195 flush_pending_session(account_id, lock);
196
197 // Remove account info from accounts_ map
198
199=== modified file 'test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp'
200--- test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp 2014-10-09 16:15:06 +0000
201+++ test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp 2014-10-10 13:41:13 +0000
202@@ -179,7 +179,7 @@
203 return result;
204 }
205
206- void invoke_callback(std::shared_ptr<OnlineAccountClient> oa_client, AccountInfo const* info, std::string const& error)
207+ void invoke_callback(std::shared_ptr<OnlineAccountClient> oa_client, AccountInfo* info, std::string const& error)
208 {
209 oa_client->p->callback(info, error);
210 }
211@@ -206,7 +206,7 @@
212 std::shared_ptr<AgAccount> account_;
213 std::shared_ptr<GMainContext> main_loop_context_;
214
215- bool got_update_;
216+ bool got_update_ = false;
217 std::mutex mutex_;
218 std::condition_variable cond_;
219
220@@ -445,34 +445,34 @@
221
222 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_none, this, std::placeholders::_1));
223 create_account();
224- wait_for_service_update();
225+ EXPECT_TRUE(wait_for_service_update());
226
227 statuses = oa_client()->get_service_statuses();
228 EXPECT_EQ(1, statuses.size());
229
230 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_enabled, this, std::placeholders::_1));
231 enable_service();
232- wait_for_service_update();
233+ EXPECT_TRUE(wait_for_service_update());
234
235 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_disabled, this, std::placeholders::_1));
236 disable_service();
237- wait_for_service_update();
238+ EXPECT_TRUE(wait_for_service_update());
239
240 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_enabled, this, std::placeholders::_1));
241 enable_service();
242- wait_for_service_update();
243+ EXPECT_TRUE(wait_for_service_update());
244
245 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_disabled, this, std::placeholders::_1));
246 disable_account();
247- wait_for_service_update();
248+ EXPECT_TRUE(wait_for_service_update());
249
250 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_enabled, this, std::placeholders::_1));
251 enable_account();
252- wait_for_service_update();
253+ EXPECT_TRUE(wait_for_service_update());
254
255 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_disabled, this, std::placeholders::_1));
256 delete_account();
257- wait_for_service_update();
258+ EXPECT_TRUE(wait_for_service_update());
259
260 statuses = oa_client()->get_service_statuses();
261 EXPECT_EQ(0, statuses.size());
262@@ -544,7 +544,7 @@
263 // Manually invoke the callback with a valid access token, which should result in service_authenticated = true
264 oa_client()->set_service_update_callback(std::bind(&OnlineAccountClientTest::service_update_auth, this, std::placeholders::_1));
265 invoke_callback(oa_client(), info.get(), "not really an error, but just to test");
266- wait_for_service_update();
267+ EXPECT_TRUE(wait_for_service_update());
268 }
269
270 int main(int argc, char **argv)

Subscribers

People subscribed via source and target branches

to all changes: