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

[Alberto Aguirre]

Avoid unlocking sesssion mutex in a different thread.

In some cases, SessionMediator needs to lock the session
while it waits for Surface::swap_buffers which may complete
asynchronously on a different thread. Since a std::mutex
cannot be used for such purposes a SemaphoreLock type is introduced.

Avoid unlocking sesssion mutex in a different thread.

In some cases, SessionMediator needs to lock the session
while it waits for Surface::swap_buffers which may complete
asynchronously on a different thread. Since a std::mutex
cannot be used for such purposes a SemaphoreLock type is introduced.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2361
http://jenkins.qa.ubuntu.com/job/mir-ci/3130/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1511
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1510
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1465
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1127
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1127/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1465
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1465/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4485
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18561

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I'm not yet sure what you're trying to express, but Semaphore(Lock) seems like the wrong name.

I recall semaphore as a type of lock with a counter. The counter is the defining feature of a semaphore. And a quick search seems to support this: http://en.wikipedia.org/wiki/Semaphore_(programming)

So I think "Semaphore" is an incorrect name.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

I am OK with the name, perhaps I would slightly prefer BinarySemaphore.

103 - auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
104 + SemaphoreUnlockIfUnwinding lock{session_lock};1

147 - auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
148 + SemaphoreUnlockIfUnwinding lock{session_lock};

The current locking scheme (using shared<unique_lock>) is safer. With the new scheme we may never release the lock if something goes wrong with the caller of the functor or the functor itself, since we have dropped RAII.

> I recall semaphore as a type of lock with a counter. The counter is the defining feature
> of a semaphore. And a quick search seems to support this:
> http://en.wikipedia.org/wiki/Semaphore_(programming)

From the bottom of the page:

A mutex is essentially the same thing as a binary semaphore and sometimes uses the
same basic implementation ... Mutexes have a concept of an owner. Only the process
that locked the mutex is supposed to unlock it. If the owner is stored by the mutex
this can be verified at runtime.

"Needs fixing" for the less safe locking scheme, looks good otherwise.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

*Needs Discussion*

I wonder whether the better fix is to ensure there is only a single IPC thread running and dispatch all activity to that thread.

review: Needs Information

[Alberto Aguirre (albaguirre)]

> *Needs Discussion*
>
> I wonder whether the better fix is to ensure there is only a single IPC thread
> running and dispatch all activity to that thread.

By default the IPC thread pool is set to 1 but it's configurable option (--ipc-thread-pool)

The other thread involved is the compositor thread, because of the completion given to surface.swap_buffers in SessionMediator::exchange_buffer (which we could make synchronous).

[Alberto Aguirre (albaguirre)]

So we have three options:

1. Use a BinarySemaphore as proposed here
2. Make SessionMediator::exchange_buffer (and others which advance the buffer) wait for the completion and execute done->Run synchronously, in which case we can use a std::mutex
3. Remove the session_mutex all together by removing the --ipc-thread-pool option and doing 2 above (so only a single IPC thread is active at any time)

[Alberto Aguirre (albaguirre)]

Well option 2,3 would block the IPC thread, so not good.

Option 4. Schedule IPC responses to the IPC thread that originated them (if we still want an IPC thread pool)

I think we can land this for now (pending addressing Alexandros comments) and then do some refactoring to allow option 4 in a separate MP.

[Alan Griffiths (alan-griffiths)]

OK.

Let's go with a variant of this scheme.

Can we avoid the dubious release semantics of SemaphoreUnlockIfUnwinding by using move capture? (Thereby proving it was right to change to C++14!)

review: Needs Fixing

[Alberto Aguirre (albaguirre)]

Well after looking at bit more at the code, it seems like the real reason for the session_mutex unlock in the completion is to avoid holding a lock when invoking IPC callbacks.

Since Surface::swap_buffers may execute the completion within, we need to detect such condition and only unlock the session_mutex then instead of using a binary semaphore.

review: Disapprove

Unmerged revisions

2361. By Alberto Aguirre on 2015-03-04

Avoid unlocking sesssion mutex in a different thread.

In some cases, SessionMediator needs to lock the session
while it waits for Surface::swap_buffers which may complete asynchronously on a different thread.
Since a std::mutex cannot be used for such purposes a SemaphoreLock type is introduced.

=== added file 'src/include/common/mir/semaphore_lock.h'
--- src/include/common/mir/semaphore_lock.h	1970-01-01 00:00:00 +0000
+++ src/include/common/mir/semaphore_lock.h	2015-03-04 20:00:48 +0000
@@ -0,0 +1,81 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored By: Alberto Aguirre <[e-mail]>
+ */
+
+#ifndef MIR_SEMAPHORE_LOCK_H_
+#define MIR_SEMAPHORE_LOCK_H_
+
+#include <boost/throw_exception.hpp>
+
+#include <exception>
+#include <mutex>
+#include <condition_variable>
+
+namespace mir
+{
+
+class SemaphoreLock
+{
+public:
+    // Make it a BasicLockable so it can be used with
+    // std::lock_guard or std::unique_lock
+    void lock()
+    {
+        std::unique_lock<decltype(mutex)> lock{mutex};
+        cv.wait(lock, [this]{ return !locked;});
+        locked = true;
+    }
+
+    void unlock()
+    {
+        std::lock_guard<decltype(mutex)> lock{mutex};
+
+        if (!locked)
+            BOOST_THROW_EXCEPTION(std::logic_error("Already unlocked"));
+
+        locked = false;
+        cv.notify_one();
+    }
+
+private:
+    std::mutex mutex;
+    std::condition_variable cv;
+    bool locked = false;
+};
+
+class SemaphoreUnlockIfUnwinding
+{
+public:
+    explicit SemaphoreUnlockIfUnwinding(SemaphoreLock& guard)
+        : guard(guard)
+    {
+        guard.lock();
+    }
+
+    ~SemaphoreUnlockIfUnwinding()
+    {
+        if (std::uncaught_exception())
+            guard.unlock();
+    }
+
+private:
+    SemaphoreLock& guard;
+};
+
+}
+
+#endif /* MIR_SEMAPHORE_LOCK_H_ */

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2015-02-13 06:12:34 +0000
+++ src/server/frontend/session_mediator.cpp	2015-03-04 20:00:48 +0000
@@ -122,7 +122,7 @@
 
     auto const session = shell->open_session(client_pid_, request->application_name(), event_sink);
     {
-        std::lock_guard<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         weak_session = session;
     }
     connection_context.handle_client_connect(session);
@@ -172,7 +172,7 @@
     google::protobuf::Closure* done)
 {
 
-    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    SemaphoreUnlockIfUnwinding lock{session_lock};
 
     auto const session = weak_session.lock();
 
@@ -243,10 +243,10 @@
     }
 
     advance_buffer(surf_id, *surface,
-        [lock, this, &surf_id, response, done, session]
+        [this, &surf_id, response, done, session]
         (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
         {
-            lock->unlock();
+            session_lock.unlock();
 
             response->mutable_buffer_stream()->mutable_id()->set_value(
                surf_id.as_value());
@@ -271,7 +271,7 @@
 {
     SurfaceId const surf_id{request->value()};
 
-    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    SemaphoreUnlockIfUnwinding lock{session_lock};
 
     auto const session = weak_session.lock();
 
@@ -283,10 +283,10 @@
     auto surface = session->get_surface(surf_id);
 
     advance_buffer(surf_id, *surface,
-        [lock, this, response, done, session]
+        [this, response, done, session]
         (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
         {
-            lock->unlock();
+            session_lock.unlock();
 
             pack_protobuf_buffer(*response, client_buffer, msg_type);
 
@@ -306,7 +306,7 @@
     mfd::ProtobufBufferPacker request_msg{const_cast<mir::protobuf::Buffer*>(&request->buffer())};
     ipc_operations->unpack_buffer(request_msg, *surface_tracker.last_buffer(surface_id));
 
-    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    SemaphoreUnlockIfUnwinding lock{session_lock};
     auto const session = weak_session.lock();
     if (!session)
         BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
@@ -316,15 +316,15 @@
     auto const& surface = session->get_surface(surface_id);
     surface->swap_buffers(
         surface_tracker.buffer_from(buffer_id),
-        [this, surface_id, lock, response, done](mg::Buffer* new_buffer)
+        [this, surface_id, response, done](mg::Buffer* new_buffer)
         {
-            lock->unlock();
-
             if (surface_tracker.track_buffer(surface_id, new_buffer))
                 pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::update_msg);
             else
                 pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::full_msg);
 
+            // Unlock until now as surface_tracker may be modified above.
+            session_lock.unlock();
             done->Run();
         });
 }
@@ -336,7 +336,7 @@
     google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
 
         auto session = weak_session.lock();
 
@@ -362,7 +362,7 @@
     google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
 
         auto session = weak_session.lock();
 
@@ -391,7 +391,7 @@
     response->set_attrib(attrib);
 
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
 
         auto session = weak_session.lock();
 
@@ -417,7 +417,7 @@
     ::google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         auto session = weak_session.lock();
 
         if (session.get() == nullptr)
@@ -536,7 +536,7 @@
     google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
 
         auto session = weak_session.lock();
 
@@ -568,7 +568,7 @@
     ::google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         auto session = weak_session.lock();
 
         if (session.get() == nullptr)
@@ -600,7 +600,7 @@
     ::google::protobuf::Closure *done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
 
         auto session = weak_session.lock();
 
@@ -626,7 +626,7 @@
     google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         auto session = weak_session.lock();
 
         if (session.get() == nullptr)
@@ -660,7 +660,7 @@
     google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         auto session = weak_session.lock();
 
         if (session.get() == nullptr)
@@ -695,7 +695,7 @@
     ::google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         auto const session = weak_session.lock();
 
         if (!session)
@@ -721,7 +721,7 @@
     ::google::protobuf::Closure* done)
 {
     {
-        std::unique_lock<std::mutex> lock(session_mutex);
+        std::lock_guard<decltype(session_lock)> lock{session_lock};
         auto const session = weak_session.lock();
 
         if (!session)

=== modified file 'src/server/frontend/session_mediator.h'
--- src/server/frontend/session_mediator.h	2015-01-21 07:34:50 +0000
+++ src/server/frontend/session_mediator.h	2015-03-04 20:00:48 +0000
@@ -20,6 +20,7 @@
 #define MIR_FRONTEND_SESSION_MEDIATOR_H_
 
 #include "display_server.h"
+#include "mir/semaphore_lock.h"
 #include "mir/frontend/connection_context.h"
 #include "mir/frontend/surface_id.h"
 #include "mir/graphics/platform_ipc_operations.h"
@@ -213,7 +214,7 @@
 
     SurfaceTracker surface_tracker;
 
-    std::mutex session_mutex;
+    SemaphoreLock session_lock;
     std::weak_ptr<Session> weak_session;
     std::weak_ptr<PromptSession> weak_prompt_session;
 };

=== modified file 'tests/unit-tests/CMakeLists.txt'
--- tests/unit-tests/CMakeLists.txt	2015-03-03 06:56:37 +0000
+++ tests/unit-tests/CMakeLists.txt	2015-03-04 20:00:48 +0000
@@ -60,6 +60,7 @@
   test_shared_library_prober.cpp
   test_lockable_callback.cpp
   test_module_deleter.cpp
+  test_semaphore_lock.cpp
 )
 
 add_subdirectory(options/)

=== added file 'tests/unit-tests/test_semaphore_lock.cpp'
--- tests/unit-tests/test_semaphore_lock.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/test_semaphore_lock.cpp	2015-03-04 20:00:48 +0000
@@ -0,0 +1,56 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored By: Alberto Aguirre <[e-mail]>
+ */
+
+#include "mir/semaphore_lock.h"
+
+#include <gtest/gtest.h>
+
+#include <exception>
+
+using namespace testing;
+
+namespace
+{
+void lock_and_throw(mir::SemaphoreLock& guard)
+{
+    mir::SemaphoreUnlockIfUnwinding lock{guard};
+    throw std::runtime_error("");
+}
+}
+
+TEST(SemaphoreLock, throws_on_unlock_if_already_unlocked)
+{
+    mir::SemaphoreLock guard;
+    EXPECT_THROW(guard.unlock(), std::logic_error);
+}
+
+TEST(SemaphoreUnlockIfUnwinding, keeps_lock_if_no_exception)
+{
+    mir::SemaphoreLock guard;
+    {
+        mir::SemaphoreUnlockIfUnwinding lock{guard};
+    }
+    EXPECT_NO_THROW(guard.unlock());
+}
+
+TEST(SemaphoreUnlockIfUnwinding, unlocks_after_exception)
+{
+    mir::SemaphoreLock guard;
+    EXPECT_THROW(lock_and_throw(guard), std::runtime_error);
+    EXPECT_THROW(guard.unlock(), std::logic_error);
+}