Mir

Merge lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration into lp://qastaging/mir

fake-event-hub-to-fake-input-server-configuration
Merge into development-branch

Proposed by Andreas Pokorny on 2015-05-12

Status:	Work in progress
Proposed branch:	lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration
Merge into:	lp://qastaging/mir
Prerequisite:	lp://qastaging/~andreas-pokorny/mir/stub-legacy-input-dispatchable
Diff against target:	677 lines (+161/-117) 10 files modified benchmarks/frame-uniformity/touch_producing_server.cpp (+12/-10) benchmarks/frame-uniformity/touch_producing_server.h (+5/-2) src/server/input/default_input_manager.cpp (+35/-23) src/server/input/default_input_manager.h (+5/-2) tests/acceptance-tests/throwback/test_client_cursor_api.cpp (+15/-15) tests/include/mir_test_framework/fake_input_server_configuration.h (+12/-22) tests/integration-tests/input/test_cursor_listener.cpp (+2/-3) tests/integration-tests/test_server_shutdown.cpp (+59/-12) tests/mir_test_framework/CMakeLists.txt (+1/-1) tests/mir_test_framework/fake_input_server_configuration.cpp (+15/-27)
To merge this branch:	bzr merge lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-06-22
Alan Griffiths			Needs Fixing on 2015-05-28
Robert Carr (community)			Approve on 2015-05-19
Kevin DuBois (community)		2015-05-12	Needs Fixing on 2015-05-14
Review via email: mp+258845@code.qastaging.launchpad.net

Commit message

Switch all tests to FakeInputServerConfiguration, and ensure that DefaultInputManagers dispatchables are unlinked from the MutliplexingDispatchable in all shutdown paths.

Description of the change

Change FakeEventHubServerConfiguration into FakeInputServerConfiguration - a server configuration that ensures that an input platform is loaded that can be used with fake input devices.

There were a few copies of FakeEventHubServerConfigration in the code, those were changed to. Hence the shutdown case: exception during user input processing was converted to. That required a cleanup of the DefaultInputManager 'stop' paths.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-12:

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

review: Needs Fixing (continuous-integration)

lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration updated on 2015-05-12

2369. By Andreas Pokorny on 2015-05-12: merge clang fix

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-12:

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

review: Approve (continuous-integration)

lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration updated on 2015-05-12

2370. By Andreas Pokorny on 2015-05-12: include not needed

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-12:

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-05-14:

183+ for (auto const platform : platforms)
const&?

123+ if (!state.compare_exchange_strong(expected, State::running))
Close to my EOD (and my brain's somewhat cooked), but the existing code doesn't look threadsafe, and this could still have an interleaving of stop() and start(). I'd guess the start and stop need to be exclusive (maybe a 'start', 'stop' and 'transitioning' state could work). If its also not threadsafe now, could we leave it as is?

review: Needs Fixing

lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration updated on 2015-05-19

2371. By Andreas Pokorny on 2015-05-13: merge prereq
2372. By Andreas Pokorny on 2015-05-19: merge prereq
2373. By Andreas Pokorny on 2015-05-19: fixing missing ref

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-05-19:

> 123+ if (!state.compare_exchange_strong(expected, State::running))
> Close to my EOD (and my brain's somewhat cooked), but the existing code
> doesn't look threadsafe, and this could still have an interleaving of stop()
> and start(). I'd guess the start and stop need to be exclusive (maybe a
> 'start', 'stop' and 'transitioning' state could work). If its also not
> threadsafe now, could we leave it as is?

Yes it is not safe against interleaving start/stop calls. This change only helps against parallel stop calls.. Looking at the code again, it also lacks exception safety. I think I should bring it on par with the start/stop behavior of MultithreadedCompositor

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-19:

PASSED: Continuous integration, rev:2373
http://jenkins.qa.ubuntu.com/job/mir-ci/3873/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2531
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/41
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2479
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/29
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/29/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2479
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2479/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5353
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20610

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

review: Approve (continuous-integration)

Revision history for this message

Robert Carr (robertcarr) wrote on 2015-05-19:

LGTM Besides the mentioned exception safety in start/stop.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-05-28:

The only blocking issue I see is the exception safety.

~~~~

One nit:

89 + std::unique_ptr<mir_test_framework::FakeInputDevice> touch_screen;

const?

~~~~

One (pre-existing) wish:

359 std::shared_ptr<mir::input::InputManager> the_input_manager() override;
360 std::shared_ptr<mir::input::InputDispatcher> the_input_dispatcher() override;
...
362 std::shared_ptr<mir::input::InputSender> the_input_sender() override;

A shame these are needed but are not customization points in mir::Server (as that would go a long way to getting rid of these "throwback" tests).

review: Needs Fixing

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-05-29:

Uh, I nearly forgot about that branch.

> One (pre-existing) wish:
>
> 359 std::shared_ptr<mir::input::InputManager> the_input_manager()
> override;
> 360 std::shared_ptr<mir::input::InputDispatcher>
> the_input_dispatcher() override;
> ...
> 362 std::shared_ptr<mir::input::InputSender> the_input_sender()
> override;
>
> A shame these are needed but are not customization points in mir::Server (as
> that would go a long way to getting rid of these "throwback" tests).

I believe none of those three are touched in throwback. I think throwback could be cleaned if we add a customization point to track the scene contents (cursor + touch vizsualiation) and the used cursor buffers. Hmm the other remaining would be focus tracking.. We might get away by testing for focus events on the client?

lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration updated on 2015-05-29

2374. By Andreas Pokorny on 2015-05-22: merge lp:mir
2375. By Andreas Pokorny on 2015-05-29: merge lp:mir
2376. By Andreas Pokorny on 2015-05-29: nits fixed additional tear down on stop

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-05-29:

hm maybe my last change was unnecessary as unique_ptr should cover that case nicely..

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-29:

FAILED: Continuous integration, rev:2376
http://jenkins.qa.ubuntu.com/job/mir-ci/3946/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2630
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/141
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2578/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/102/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2578
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2578/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5429/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20841

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

review: Needs Fixing (continuous-integration)

lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration updated on 2015-05-30

2377. By Andreas Pokorny on 2015-05-30: revert additional stack unwinding code

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-05-30:

PASSED: Continuous integration, rev:2377
http://jenkins.qa.ubuntu.com/job/mir-ci/3953/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2638
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/149
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2586
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/109
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/109/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2586
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2586/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5436
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20850

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-06-01:

> > One (pre-existing) wish:
> >
> > 359 std::shared_ptr<mir::input::InputManager> the_input_manager()
> > override;
> > 360 std::shared_ptr<mir::input::InputDispatcher>
> > the_input_dispatcher() override;
> > ...
> > 362 std::shared_ptr<mir::input::InputSender> the_input_sender()
> > override;
>
> I believe none of those three are touched in throwback. I think throwback
> could be cleaned if we add a customization point to track the scene contents
> (cursor + touch vizsualiation) and the used cursor buffers. Hmm the other
> remaining would be focus tracking.. We might get away by testing for focus
> events on the client?

You say they are "not touched" but the tests use this configuration and this configuration implements these overrides. So touched or not they are used.

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-06-02:

Especially given that start/stop isn't on a critical path, guarding against interleaving start/stop calls seems like something thats easy to do with a mutex... but if that's not the goal, it seems okay to guard against many stop() or start() calls this way. So I guess still needs fixing on the exception safety.

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-06-10:

So..is this work in progress? Or still needs review?

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-06-11:

> So..is this work in progress? Or still needs review?

"Needs fixing" on the exception safety.

lp://qastaging/~andreas-pokorny/mir/fake-event-hub-to-fake-input-server-configuration updated on 2015-06-22

2378. By Andreas Pokorny on 2015-06-21: merged lp:mir
2379. By Andreas Pokorny on 2015-06-22: Mutually exclude multiple start and stop executions

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-06-22:

PASSED: Continuous integration, rev:2379
http://jenkins.qa.ubuntu.com/job/mir-ci/4165/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2948
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/463
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2896
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/321
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/321/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2896
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2896/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5720
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21371

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-06-23:

Am I missing something?

AFAICS this has been blocked for four weeks over on the same issue (exception safety) and the latest revision does not address this.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-06-23:

> Am I missing something?
>
> AFAICS this has been blocked for four weeks over on the same issue (exception
> safety) and the latest revision does not address this.

No you are not. I returned to the branch today.. and I am still on it.