Mir

Merge lp:~afrantzis/mir/fix-1679591-display-config-observer-race into lp:mir

  1. fix-1679591-display-config-observer-race
  2. Merge into development-branch
Proposed by Alexandros Frantzis
Status: Work in progress
Proposed branch: lp:~afrantzis/mir/fix-1679591-display-config-observer-race
Merge into: lp:mir
Diff against target: 95 lines (+57/-3)
2 files modified
src/server/scene/mediating_display_changer.cpp (+6/-1)
tests/unit-tests/scene/test_mediating_display_changer.cpp (+51/-2)
To merge this branch: bzr merge lp:~afrantzis/mir/fix-1679591-display-config-observer-race
Reviewer Review Type Date Requested Status
Alexandros Frantzis (community) Needs Information
Mir CI Bot continuous-integration Needs Fixing
Cemil Azizoglu (community) Approve
Alberto Aguirre (community) Approve
Chris Halse Rogers Approve
Review via email: mp+321870@code.launchpad.net

Commit message

server: Emit the DisplayConfigurationObserver initial_configuration notification through the server action queue

Description of the change

server: Use the server action queue for all DisplayConfigurationObserver notifications

The DisplayConfigurationObserver::initial_configuration() notification is currently emitted during construction of the MediatingDisplayChanger object, which happens at some indeterminate time before the server starts. This makes it difficult to catch this event, since the handler for the observer needs to be registered before this indeterminate time point.

This problem is the root cause of the linked bug #1679591. In that bug USC misses the initial config event because it registers interest in the observer in the server init callback, which is an intuitively correct, but it's too late in terms of the current implementation.

This MP changes the notification to emitted through the event loop. Thus the notification is guaranteed to be emitted after the server starts running, which is: 1. a well defined and 2. the expected behavior from the user's point of view.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4140
https://mir-jenkins.ubuntu.com/job/mir-ci/3302/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4464/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4579
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4569
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4569
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4569
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4569
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4496/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4496/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4496/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4496/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4496/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4496/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4496/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3302/rebuild

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Yup, this looks sensible.

review: Approve
Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Hah! For anyone else wondering why this doesn't work *already*, because the observer delegates all activations to the MainLoop anyway...

It's because it only does that for observers registered *at observation time*, and observation time is too early. Phew!

review: Approve
Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4141
https://mir-jenkins.ubuntu.com/job/mir-ci/3311/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4476/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4591
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4580
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4580
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4580
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4580
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4507
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4507/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4507/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4507/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4507/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4507
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4507/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4507
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4507/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3311/rebuild

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Makes sense.

review: Approve
Reply
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

ok

review: Approve
Reply
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/1268/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4494/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/1348/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4612
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4601
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4601
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4601
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4601
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4525
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4525/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4525/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4525/console
    ABORTED: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4525/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4525/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4525
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4525/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4525
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4525/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Hmm, these look suspicious, I need to investigate more:

FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4525/console
FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4525/console

Do not land yet.

review: Needs Information
Reply

Unmerged revisions

4141. By Alexandros Frantzis

tests: Fix gmock expectation complaints and leak

4140. By Alexandros Frantzis

server: Emit the DisplayConfigurationObserver initial_configuration notification through the server action queue

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/scene/mediating_display_changer.cpp'
2--- src/server/scene/mediating_display_changer.cpp 2017-03-13 08:12:52 +0000
3+++ src/server/scene/mediating_display_changer.cpp 2017-04-05 08:07:30 +0000
4@@ -177,7 +177,12 @@
5 });
6 });
7
8- observer->initial_configuration(base_configuration_);
9+ server_action_queue->enqueue(
10+ this,
11+ [this]
12+ {
13+ this->observer->initial_configuration(base_configuration_);
14+ });
15 }
16
17 void ms::MediatingDisplayChanger::configure(
18
19=== modified file 'tests/unit-tests/scene/test_mediating_display_changer.cpp'
20--- tests/unit-tests/scene/test_mediating_display_changer.cpp 2017-03-16 03:23:51 +0000
21+++ tests/unit-tests/scene/test_mediating_display_changer.cpp 2017-04-05 08:07:30 +0000
22@@ -683,7 +683,7 @@
23 auto const conf = std::make_shared<mtd::NullDisplayConfiguration>();
24 auto const session1 = std::make_shared<mtd::StubSession>();
25 auto const session2 = std::make_shared<mtd::StubSession>();
26- MockServerActionQueue mock_server_action_queue;
27+ NiceMock<MockServerActionQueue> mock_server_action_queue;
28
29 stub_session_container.insert_session(session1);
30 stub_session_container.insert_session(session2);
31@@ -737,7 +737,7 @@
32 auto const conf = std::make_shared<mtd::NullDisplayConfiguration>();
33 auto const active_session = std::make_shared<mtd::StubSession>();
34 auto const inactive_session = std::make_shared<mtd::StubSession>();
35- MockServerActionQueue mock_server_action_queue;
36+ NiceMock<MockServerActionQueue> mock_server_action_queue;
37
38 stub_session_container.insert_session(active_session);
39 stub_session_container.insert_session(inactive_session);
40@@ -888,6 +888,55 @@
41 changer->set_base_configuration(mt::fake_shared(conf));
42 }
43
44+TEST_F(MediatingDisplayChangerTest, notifies_observer_on_initial_configuration_through_action_queue)
45+{
46+ using namespace testing;
47+
48+ struct MockDisplayConfigurationObserver : StubDisplayConfigurationObserver
49+ {
50+ MOCK_METHOD1(initial_configuration,
51+ void (std::shared_ptr<mg::DisplayConfiguration const> const&));
52+ } display_configuration_observer;
53+
54+ struct DelayedServerActionQueue : mir::ServerActionQueue
55+ {
56+ void enqueue(void const* /*owner*/, mir::ServerAction const& action) override
57+ {
58+ actions.push_back(action);
59+ }
60+
61+ void pause_processing_for(void const* /*owner*/) override {}
62+ void resume_processing_for(void const* /*owner*/) override {}
63+
64+ void process()
65+ {
66+ for (auto const& action : actions)
67+ action();
68+ }
69+
70+ std::vector<mir::ServerAction> actions;
71+ };
72+
73+ DelayedServerActionQueue delayed_server_action_queue;
74+
75+ EXPECT_CALL(display_configuration_observer, initial_configuration(_)).Times(0);
76+
77+ changer = std::make_shared<ms::MediatingDisplayChanger>(
78+ mt::fake_shared(mock_display),
79+ mt::fake_shared(mock_compositor),
80+ mt::fake_shared(mock_conf_policy),
81+ mt::fake_shared(stub_session_container),
82+ mt::fake_shared(session_event_sink),
83+ mt::fake_shared(delayed_server_action_queue),
84+ mt::fake_shared(display_configuration_observer),
85+ mt::fake_shared(alarm_factory));
86+
87+ Mock::VerifyAndClearExpectations(&display_configuration_observer);
88+
89+ EXPECT_CALL(display_configuration_observer, initial_configuration(_));
90+ delayed_server_action_queue.process();
91+}
92+
93 TEST_F(MediatingDisplayChangerTest, notifies_session_on_preview_base_configuration)
94 {
95 using namespace testing;

Subscribers

People subscribed via source and target branches