Mir

Merge lp:~alan-griffiths/mir/workaround-1525003 into lp:mir

workaround-1525003
Merge into development-branch

Proposed by Alan Griffiths on 2016-02-01

Status:

Merged

Approved by:

Alan Griffiths on 2016-02-03

Approved revision:

no longer in the source branch.

Merged at revision:

3291

Proposed branch:

lp:~alan-griffiths/mir/workaround-1525003

Merge into:

lp:mir

Diff against target:

31 lines (+9/-4)

1 file modified

tests/acceptance-tests/throwback/test_client_cursor_api.cpp (+9/-4)

To merge this branch:

bzr merge lp:~alan-griffiths/mir/workaround-1525003

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2016-02-03
Alexandros Frantzis (community)			Approve on 2016-02-03
Mir CI Bot	continuous-integration		Approve on 2016-02-03
Kevin DuBois (community)			Approve on 2016-02-02
Daniel van Vugt			Needs Information on 2016-02-02
Alberto Aguirre (community)		2016-02-01	Approve on 2016-02-01
Review via email: mp+284626@code.launchpad.net

Commit message

tests: Add a small delay to ensure the TestClientCursorAPI.cursor_passed_through_nested_server test conditions are set up correctly.

Description of the change

tests: Add a small delay to ensure the TestClientCursorAPI.cursor_passed_through_nested_server test conditions are set up correctly.

I'd really like a better approach, but I've not managed to identify a better approach, and chose to limit the time spent on this.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-02-01:

PASSED: Continuous integration, rev:3281
https://mir-jenkins.ubuntu.com/job/mir-ci/201/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/202/console

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-02-01:

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2016-02-01:

Yep I didn't see a better way either.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-02:

mir_surface_set_state is asynchronous and you're not waiting for it. That's a feature which is sometimes useful, but does it put your expectation at risk?

14 + mir_surface_set_state(surface, mir_surface_state_fullscreen);
15 auto conf = mir_cursor_configuration_from_name(mir_disabled_cursor_name);
16 - mir_wait_for(mir_surface_configure_cursor(surface, conf));
17 + mir_surface_configure_cursor(surface, conf);
18 mir_cursor_configuration_destroy(conf);

In theory at least our client API allows calls to complete out of order. Maybe none do just yet, but that's not something we should assume.

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-02:

> mir_surface_set_state is asynchronous and you're not waiting for it. That's a
> feature which is sometimes useful, but does it put your expectation at risk?

No. Waiting for the RPC call to return doesn't add anything toward the expectation.

> In theory at least our client API allows calls to complete out of order. Maybe
> none do just yet, but that's not something we should assume.

I disagree with that theory. The server should handle calls in the order the client makes them.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-02-02:

lgtm, barring using some more rigging in the server side (which iirc, is tricky in throwback/)

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-02-03:

I'm still not convinced this is theoretically correct. If the "mir_surface_set_state(surface, mir_surface_state_fullscreen);" contributes anything to the test then it will only do so if the request is completed in time. If the surface is not full screen before we do cursor tinkering then either some test may fail, or you didn't need the fullscreening in the first place.

I know that the server presently completes most requests in order, but don't think we document that, or should document that.

Needs another reviewer.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2016-02-03:

Given our current level of explicit guarantees (i.e. none), I agree with Daniel about this.

There are two questions of ordering/timing in the code:

1. Whether mir_surface_configure_cursor() will be processed by the server before mir_surface_set_state().
2. Whether mir_surface_configure_cursor() will have been processed by the server by the time we release the surface/connection.

We don't provide explicit guarantees for either of the above (AFAIK, please correct me if we do), although one could say that at least (1) is the expected behavior. We either need to make explicit guarantees about ordering (and ensure we keep our word == tests), or wait for each operation to complete.

Since providing the guarantees is outside the scope of this MP, I propose we just reinstated the waits for now, and have a discussion about the ordering topic.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-03:

> Since providing the guarantees is outside the scope of this MP, I propose we
> just reinstated the waits for now, and have a discussion about the ordering
> topic.

OK, but the mir_wait_for() calls do not address either of your points.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-02-03:

> Given our current level of explicit guarantees (i.e. none), I agree with
> Daniel about this.
>
> There are two questions of ordering/timing in the code:
>
> 1. Whether mir_surface_configure_cursor() will be processed by the server
> before mir_surface_set_state().

I say this is expected behavior - and would be surprised if we ever change that. Doing so would invalidate other tests - e.g. ClientLibrary.can_set_surface_state.

Further, if the server can re-order the requests, then waiting for the request submission to complete doesn't give any guarantee that the request has been handled.

> 2. Whether mir_surface_configure_cursor() will have been processed by the
> server by the time we release the surface/connection.

We wait (expectations_satisfied.wait_for_at_most_seconds(60);) for the server to process the call before closing the client.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-02-03:

PASSED: Continuous integration, rev:3282
https://mir-jenkins.ubuntu.com/job/mir-ci/216/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/217/console

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

review: Approve (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2016-02-03:

> Further, if the server can re-order the requests, then waiting for the request submission
> to complete doesn't give any guarantee that the request has been handled.

Why not? (do you mean handled with success?)

> We wait (expectations_satisfied.wait_for_at_most_seconds(60);) for the server to process the
> call before closing the client.

Fair enough.

review: Approve

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2016-02-03:

The main issue is waiting for the nested display surface to become exposed and focused; there's currently no good way to capture those events. It's important because the nested server will eventually end up doing mgn::MirClientHostConnection::hide_cursor which will race with the surface being exposed+focused

The mir_surface_set_state and mir_surface_configure_cursor will happen in order at the server (by default ipc threads is 1).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-02-03:

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

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alan Griffiths

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'tests/acceptance-tests/throwback/test_client_cursor_api.cpp'
 --- tests/acceptance-tests/throwback/test_client_cursor_api.cpp	2016-01-29 08:18:22 +0000
 +++ tests/acceptance-tests/throwback/test_client_cursor_api.cpp	2016-02-03 15:58:01 +0000
@@ -463,6 +463,11 @@
      void setup_cursor(MirSurface* surface) override
+     {
++        // Workaround race condition (lp:1525003). I've tried, but I've not
++        // found a better way to ensure that the host Mir server is "ready"
++        // for the test logic. - alan_g
++        std::this_thread::sleep_for(std::chrono::milliseconds(20));
++
          mir_wait_for(mir_surface_set_state(surface, mir_surface_state_fullscreen));
          auto conf = mir_cursor_configuration_from_name(mir_disabled_cursor_name);
          mir_wait_for(mir_surface_configure_cursor(surface, conf));
@@ -483,11 +488,11 @@
          .WillOnce(mt::WakeUp(&expectations_satisfied));
      { // Ensure we finalize the client prior stopping the nested server
--    FullscreenDisabledCursorClient client{nested_mir.new_connection(), client_name_1};
--    client.run();
++        FullscreenDisabledCursorClient client{nested_mir.new_connection(), client_name_1};
++        client.run();
--    expectations_satisfied.wait_for_at_most_seconds(60);
--    expect_client_shutdown();
++        expectations_satisfied.wait_for_at_most_seconds(60);
++        expect_client_shutdown();
+     }
      nested_mir.stop_server();