Mir

Merge lp:~albaguirre/mir/fix-1481034 into lp:mir

fix-1481034
Merge into development-branch

Proposed by Alberto Aguirre on 2015-08-03

Status:

Merged

Approved by:

Alberto Aguirre on 2015-08-04

Approved revision:

2808

Merged at revision:

2808

Proposed branch:

lp:~albaguirre/mir/fix-1481034

Merge into:

lp:mir

Diff against target:

44 lines (+9/-9)

1 file modified

src/server/input/default_input_manager.cpp (+9/-9)

To merge this branch:

bzr merge lp:~albaguirre/mir/fix-1481034

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2015-08-04
Alan Griffiths		2015-08-03	Approve on 2015-08-04
Review via email: mp+266781@code.launchpad.net

Commit Message

input: Avoid throwing exception while unwinding input thread

The input thread exception handler and the the enqueued action can race to set a std::promise during start.
Have the promise be owned and set by the starting action only instead.

Description of the Change

input: Avoid throwing exception while unwinding input thread

The input thread exception handler and the the enqueued action can race to set a std::promise during start.
Have the promise be owned and set by the starting action only instead.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-03:

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

review: Approve (continuous-integration)

Chris Halse Rogers (raof) wrote on 2015-08-04:

Hah! I happen to have also run into and fixed this in the glib-mainloop-deadlock branch.

I think http://bazaar.launchpad.net/~raof/mir/fix-deadlock-in-glib-alarm/revision/2802 is a cleaner fix. There's no reason for the exception handler sent to ThreadedDispatcher to touch the started_promise at all, as ~promise() sets the exception iff a value hasn't been set and we're already going to mir::terminate_with_current_exception().

Alberto Aguirre (albaguirre) wrote on 2015-08-04:

@Chris, yeah I'll cherry pick your fix in here. Thanks!

lp:~albaguirre/mir/fix-1481034 updated on 2015-08-04

2807. By Chris Halse Rogers on 2015-08-04: Use Chris' fix instead

Alan Griffiths (alan-griffiths) wrote on 2015-08-04:

24 +
25 + /* Emulate move-semantics for started_promise.
26 + *
27 + * We need the starting-lambda to own started_promise so that it is guaranteed that
28 + * started_future gets signalled; either by ->set_value in the success path or
29 + * by the destruction of started_promise generating a broken_promise exception.
30 + */
31 + started_promise.reset();

Couldn't we just do a move capture in the lambda expression?

queue->enqueue([this, promise = std::move(started_promise)]()

review: Needs Information

PS Jenkins bot (ps-jenkins) wrote on 2015-08-04:

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

review: Approve (continuous-integration)

lp:~albaguirre/mir/fix-1481034 updated on 2015-08-04

2808. By Alberto Aguirre on 2015-08-04: Use move capture instead

Alberto Aguirre (albaguirre) wrote on 2015-08-04:

> Couldn't we just do a move capture in the lambda expression?
>
> queue->enqueue([this, promise = std::move(started_promise)]()

Yep.

Alan Griffiths (alan-griffiths) wrote on 2015-08-04:

LGTM

review: Approve

PS Jenkins bot (ps-jenkins) wrote on 2015-08-04:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4472/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:

Alberto Aguirre

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'src/server/input/default_input_manager.cpp'
 --- src/server/input/default_input_manager.cpp	2015-07-31 08:20:19 +0000
 +++ src/server/input/default_input_manager.cpp	2015-08-04 16:56:59 +0000
@@ -86,32 +86,32 @@
      legacy_dispatchable->start();
--    auto const started_promise = std::make_shared<std::promise<void>>();
--    auto const weak_started_promise = std::weak_ptr<std::promise<void>>(started_promise);
++    auto started_promise = std::make_shared<std::promise<void>>();
      auto started_future = started_promise->get_future();
--    queue->enqueue([this,weak_started_promise]()
++    /*
++     * We need the starting-lambda to own started_promise so that it is guaranteed that
++     * started_future gets signalled; either by ->set_value in the success path or
++     * by the destruction of started_promise generating a broken_promise exception.
++     */
++    queue->enqueue([this,promise = std::move(started_promise)]()
+                    {
                          start_platforms();
                          // TODO: Udev monitoring is still not separated yet - an initial scan is necessary to open
                          // devices, this will be triggered through the first call to dispatch->InputReader->loopOnce.
                          legacy_dispatchable->dispatch(dispatch::FdEvent::readable);
--                        auto const started_promise =
--                            std::shared_ptr<std::promise<void>>(weak_started_promise);
--                        started_promise->set_value();
++                        promise->set_value();
                     });
      input_thread = std::make_unique<dispatch::ThreadedDispatcher>(
          "Mir/Input Reader",
          multiplexer,
--        [this,weak_started_promise]()
++        [this]()
+         {
              stop_platforms();
              multiplexer->remove_watch(queue);
              multiplexer->remove_watch(legacy_dispatchable);
              state = State::stopped;
--            if (auto started_promise = weak_started_promise.lock())
--                started_promise->set_exception(std::current_exception());
              mir::terminate_with_current_exception();
          });