Merge lp:~alan-griffiths/mir/fix-1334287 into lp:mir

[Alan Griffiths]

server: Avoid race condition in AlarmImpl

Avoid race condition in AlarmImpl

The problem in lp:1334287 is that AlarmImpl doesn't meet this guarantee.

    /**
     * \note Destruction of the Alarm guarantees that the callback will not subsequently be called
     */
    virtual ~Alarm() = default;

So, IIUC AlarmImpl::update_timer()'s lambda grabs a "data" pointer fiddles about and *then releases its lock*.

Then (on some other thread) the TimeoutFrameDroppingPolicy is deleted which destroys the Alarm/AlarmImpl which calls cancel() and sets data->state to "cancelled".

Back on the original thread and regardless of ~AlarmImpl() and AlarmImpl::cancel() having been called, data->callback() is invoked and chaos ensues.

[Cemil Azizoglu (cemil-azizoglu)]

Looks good.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

Unless I am missing something TimeoutFramedroppingPolicy reschedules the alarm (which tries to lock too) while handling the timer callback.

see timeout_frame_dropping_policy_factory.cpp:59

Maybe we have to switch to a recursive mutex as a bandaid?

I try to distill a small solution from the split main loop MP that works without a mutex.. but still trying.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Unless I am missing something TimeoutFramedroppingPolicy reschedules the alarm
> (which tries to lock too) while handling the timer callback.
>
> see timeout_frame_dropping_policy_factory.cpp:59

Ack. But that raises the question of why no test hangs as a result.

> Maybe we have to switch to a recursive mutex as a bandaid?

Or separate locks for state that needs to be locked separately.

> I try to distill a small solution from the split main loop MP that works
> without a mutex.. but still trying.

I'm trying to imagine how you can hope to meet the "can't be called after" guarantee without a lock around the call. Or do you plan to relax that guarantee?

[Alan Griffiths (alan-griffiths)]

Switched to a recursive mutex to allow calls into the alarm from the callback.

[Andreas Pokorny (andreas-pokorny)]

hm maybe because the reconfiguration attempt is conditional inside the frame dropping policy it did not trigger inside the tests..

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1724
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2001/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/703
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/709
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/703
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/522/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/519
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/519/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2073
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2073/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1875
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8844

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2001/rebuild

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

okay

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1726
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2003/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/706
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/712
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/706
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/524
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/524/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/521
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/521/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2085
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2085/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1878
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8857

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2003/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/800/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/709
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/715
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/709
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/184/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/184
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/184/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2101
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2101/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1881
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8870

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Build timed out (after 240 minutes). Marking the build as failed.
> java.lang.InterruptedException
> Build step 'Debian PBuilder NG' marked build as failure
> ERROR:pbuilderjenkins:Error during build execution

Is it my imagination or are we getting more strange timeouts. Is it CI or us?

Re-approving...

[Alan Griffiths (alan-griffiths)]

> hm maybe because the reconfiguration attempt is conditional inside the frame
> dropping policy it did not trigger inside the tests..

Which implies the code wasn't written to pass a test. :(

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/server/asio_main_loop.cpp'
--- src/server/asio_main_loop.cpp	2014-06-24 10:22:31 +0000
+++ src/server/asio_main_loop.cpp	2014-06-25 16:35:46 +0000
@@ -344,15 +344,42 @@
         {
         }
 
-        mutable std::mutex m;
-        std::function<void(void)> callback;
+        std::recursive_mutex m;
+        std::function<void(void)> const callback;
         State state;
     };
 
     ::deadline_timer timer;
-    std::shared_ptr<InternalState> data;
+    std::shared_ptr<InternalState> const data;
+    std::function<void(boost::system::error_code const& ec)> const handler;
+
+    friend auto make_handler(std::weak_ptr<InternalState> possible_data)
+    -> std::function<void(boost::system::error_code const& ec)>;
 };
 
+auto make_handler(std::weak_ptr<AlarmImpl::InternalState> possible_data)
+-> std::function<void(boost::system::error_code const& ec)>
+{
+    // Awkwardly, we can't stop the async_wait handler from being called
+    // on a destroyed AlarmImpl. This means we need to wedge a weak_ptr
+    // into the async_wait callback.
+    return [possible_data](boost::system::error_code const& ec)
+    {
+        if (!ec)
+        {
+            if (auto data = possible_data.lock())
+            {
+                std::unique_lock<decltype(data->m)> lock(data->m);
+                if (data->state == mir::time::Alarm::pending)
+                {
+                    data->state = mir::time::Alarm::triggered;
+                    data->callback();
+                }
+            }
+        }
+    };
+}
+
 AlarmImpl::AlarmImpl(boost::asio::io_service& io,
                      std::chrono::milliseconds delay,
                      std::function<void ()> callback)
@@ -372,7 +399,8 @@
 AlarmImpl::AlarmImpl(boost::asio::io_service& io,
                      std::function<void(void)> callback)
     : timer{io},
-      data{std::make_shared<InternalState>(callback)}
+      data{std::make_shared<InternalState>(callback)},
+      handler{make_handler(data)}
 {
     data->state = triggered;
 }
@@ -416,25 +444,8 @@
 
 void AlarmImpl::update_timer()
 {
+    timer.async_wait(handler);
     std::lock_guard<decltype(data->m)> lock(data->m);
-    // Awkwardly, we can't stop the async_wait handler from being called
-    // on a destroyed AlarmImpl. This means we need to wedge a shared_ptr
-    // into the async_wait callback.
-    std::weak_ptr<InternalState> possible_data = data;
-    timer.async_wait([possible_data](boost::system::error_code const& ec)
-    {
-        auto data = possible_data.lock();
-        if (!data)
-            return;
-
-        std::unique_lock<decltype(data->m)> lock(data->m);
-        if (!ec && data->state == pending)
-        {
-            data->state = triggered;
-            lock.unlock();
-            data->callback();
-        }
-    });
     data->state = pending;
 }
 }

=== modified file 'src/server/compositor/timeout_frame_dropping_policy_factory.cpp'
--- src/server/compositor/timeout_frame_dropping_policy_factory.cpp	2014-05-22 20:48:20 +0000
+++ src/server/compositor/timeout_frame_dropping_policy_factory.cpp	2014-06-25 16:35:46 +0000
@@ -41,7 +41,7 @@
 
 private:
     std::chrono::milliseconds const timeout;
-    std::unique_ptr<mir::time::Alarm> alarm;
+    std::unique_ptr<mir::time::Alarm> const alarm;
     std::atomic<unsigned int> pending_swaps;
 };
 
@@ -49,15 +49,15 @@
                                                        std::chrono::milliseconds timeout,
                                                        std::function<void(void)> drop_frame)
     : timeout{timeout},
+      alarm{timer->create_alarm([this, drop_frame]
+        {
+            assert(pending_swaps.load() > 0);
+            drop_frame();
+            if (--pending_swaps > 0)
+                alarm->reschedule_in(this->timeout);
+        })},
       pending_swaps{0}
 {
-    alarm = timer->create_alarm([this, drop_frame]
-    {
-       assert(pending_swaps.load() > 0);
-       drop_frame();
-       if (--pending_swaps > 0)
-           alarm->reschedule_in(this->timeout);
-    });
 }
 
 void TimeoutFrameDroppingPolicy::swap_now_blocking()