Mir

Merge lp://qastaging/~alan-griffiths/mir/fix-1386185 into lp://qastaging/mir

  1. fix-1386185
  2. Merge into development-branch
Proposed by Alan Griffiths
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 2006
Proposed branch: lp://qastaging/~alan-griffiths/mir/fix-1386185
Merge into: lp://qastaging/mir
Diff against target: 162 lines (+25/-42)
5 files modified
src/client/mir_connection.cpp (+24/-17)
src/client/mir_connection.h (+1/-0)
tests/integration-tests/client/test_client_render.cpp (+0/-10)
tests/integration-tests/test_protobuf.cpp (+0/-7)
tests/mir_test_framework/using_stub_client_platform.cpp (+0/-8)
To merge this branch: bzr merge lp://qastaging/~alan-griffiths/mir/fix-1386185
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alberto Aguirre (community) Approve
Kevin DuBois (community) Approve
Robert Carr (community) Approve
Review via email: mp+239743@code.qastaging.launchpad.net

Commit message

client: stop the default_lifecycle_event_handler raising SIGHUP while disconnecting.

Description of the change

client: stop the default_lifecycle_event_handler raising SIGHUP while disconnecting.

Also removed workarounds scattered through the test code

Note: it looks as though a similar change should be made to MirClientSurfaceTest - but that leads to a race condition when closing down. As it wasn't obvious how to resolve that I've left it alone.

To post a comment you must log in.
Revision history for this message
Robert Carr (robertcarr) wrote :

l35 Lambda alias is kind of weird when invoked immediately. Is there a reason?

review: Needs Information
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> l35 Lambda alias is kind of weird when invoked immediately. Is there a reason?

34 + bool const expect_connection_lost = [&]
35 + {
36 + std::lock_guard<decltype(mutex)> lock(mutex);
37 + return disconnecting;
38 + }();

To get the initial value while holding a lock. You could write this:

    bool expect_connection_lost;
    {
        std::lock_guard<decltype(mutex)> lock(mutex);
        expect_connection_lost = disconnecting;
    }

But that's less clear that it's about initializing expect_connection_lost.

Or:

    bool const expect_connection_lost{(std::lock_guard<decltype(mutex)>(mutex), disconnecting)};

Which I thought a bit too cryptic.

But I'll use whichever alternative the panel approves.

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

FAILED: Continuous integration, rev:2003
http://jenkins.qa.ubuntu.com/job/mir-ci/1916/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2258/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2265
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2193/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/256
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/256/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1130/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/1916/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

Hmm ok I guess it's ok as is. Maybe a case for an atomic though?

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> Hmm ok I guess it's ok as is. Maybe a case for an atomic though?

That works in isolation. But there's the header file comment...

    std::mutex mutex; // Protects all members of *this (except release_wait_handles)

...which would not get any clearer.

Revision history for this message
Robert Carr (robertcarr) wrote :

Forgot to approve after discussion on IRC.

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

looks okay to me

review: Approve
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Ok, I suppose this is a more pragmatic approach than https://code.launchpad.net/~raof/mir/dont-kill-the-poor-clients/+merge/232971

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

PASSED: Continuous integration, rev:2005
http://jenkins.qa.ubuntu.com/job/mir-ci/1919/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2269
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2197
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/259
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/259/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1134
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1134/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3178
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15114

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/1919/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches