Merge lp:~marcustomlinson/unity-scope-click/lp-1591422 into lp:unity-scope-click

Proposed by Marcus Tomlinson on 2016-06-14
Status: Merged
Approved by: dobey on 2016-06-14
Approved revision: 463
Merged at revision: 463
Proposed branch: lp:~marcustomlinson/unity-scope-click/lp-1591422
Merge into: lp:unity-scope-click
Diff against target: 56 lines (+20/-4)
2 files modified
libclickscope/click/preview.cpp (+10/-2)
libclickscope/click/ubuntuone_credentials.cpp (+10/-2)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scope-click/lp-1591422
Reviewer Review Type Date Requested Status
dobey (community) 2016-06-14 Approve on 2016-06-14
PS Jenkins bot continuous-integration Pending
Review via email: mp+297269@code.launchpad.net

Commit message

Ignore promise_already_satisfied exceptions thrown from getCredentials() signal handlers

To post a comment you must log in.
Marcus Tomlinson (marcustomlinson) wrote :

Looking at:

    https://errors.ubuntu.com/problem/f271ff40a203e6d22ab0bf36beb8f6f6adddb90a,

we can see that the crash originates from:

    ubuntuone_credentials.cpp:63 -> "promise.set_value(token);"

Digging a little deeper into the future source, _M_set_result() seems to only throw std::future_error for the promise_already_satisfied condition. This unhandled exception is tearing down the scope.

We have 2 places in code where getCredentials() is called:
    click::CredentialsService::getToken()
    & InstalledPreview::get_consumer_key()

I suspect that during "QCoreApplication::processEvents();" in click::CredentialsService::getToken(), we may risk processing 2 getCredentials() calls in parallel, causing multiple callbacks into the signal handler.

This change adds a safety net which may or may not make the issue go away completely, but it certainly shouldn't make things any worse.

dobey (dobey) wrote :

The basics of this look sound, but would be nice if the statements were structured consistent with the rest of the code, like so:

try {
    promise.set_value();
} catch (const std::future_error&) {
    // Ignore promise_already_satisfied
}

review: Needs Fixing
463. By Marcus Tomlinson on 2016-06-14

Fixed formatting

Marcus Tomlinson (marcustomlinson) wrote :

> The basics of this look sound, but would be nice if the statements were
> structured consistent with the rest of the code, like so:
>
> try {
> promise.set_value();
> } catch (const std::future_error&) {
> // Ignore promise_already_satisfied
> }

Sure, done.

dobey (dobey) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libclickscope/click/preview.cpp'
2--- libclickscope/click/preview.cpp 2016-06-01 13:36:55 +0000
3+++ libclickscope/click/preview.cpp 2016-06-14 17:24:57 +0000
4@@ -833,13 +833,21 @@
5 [&promise, &sso](const UbuntuOne::Token& token) {
6 qDebug() << "Credentials found";
7 sso.clear();
8- promise.set_value(token.consumerKey().toStdString());
9+ try {
10+ promise.set_value(token.consumerKey().toStdString());
11+ } catch (const std::future_error&) {
12+ // Ignore promise_already_satisfied
13+ }
14 });
15 QObject::connect(sso.data(), &click::CredentialsService::credentialsNotFound,
16 [&promise, &sso]() {
17 qDebug() << "No credentials found";
18 sso.clear();
19- promise.set_value("");
20+ try {
21+ promise.set_value("");
22+ } catch (const std::future_error&) {
23+ // Ignore promise_already_satisfied
24+ }
25 });
26
27 sso->getCredentials();
28
29=== modified file 'libclickscope/click/ubuntuone_credentials.cpp'
30--- libclickscope/click/ubuntuone_credentials.cpp 2016-05-10 13:42:12 +0000
31+++ libclickscope/click/ubuntuone_credentials.cpp 2016-06-14 17:24:57 +0000
32@@ -60,14 +60,22 @@
33 &u1::SSOService::credentialsFound,
34 [this, &promise](const u1::Token& token) {
35 emit credentialsFound(_token);
36- promise.set_value(token);
37+ try {
38+ promise.set_value(token);
39+ } catch (const std::future_error&) {
40+ // Ignore promise_already_satisfied
41+ }
42 });
43 auto notfound = QObject::connect(ssoService.data(),
44 &u1::SSOService::credentialsNotFound,
45 [this, &promise]() {
46 qWarning() << "No Ubuntu One token found.";
47 emit credentialsNotFound();
48- promise.set_value(u1::Token());
49+ try {
50+ promise.set_value(u1::Token());
51+ } catch (const std::future_error&) {
52+ // Ignore promise_already_satisfied
53+ }
54 });
55
56 getCredentials();

Subscribers

People subscribed via source and target branches