Mir

Merge lp:~raof/mir/make-default-surface-store-threadsafe into lp:mir

Proposed by Chris Halse Rogers on 2017-08-08
Status: Merged
Approved by: Alan Griffiths on 2017-08-14
Approved revision: 4222
Merged at revision: 4226
Proposed branch: lp:~raof/mir/make-default-surface-store-threadsafe
Merge into: lp:mir
Diff against target: 39 lines (+4/-0)
2 files modified
src/server/shell/default_persistent_surface_store.cpp (+2/-0)
src/server/shell/default_persistent_surface_store.h (+2/-0)
To merge this branch: bzr merge lp:~raof/mir/make-default-surface-store-threadsafe
Reviewer Review Type Date Requested Status
Alan Griffiths 2017-08-08 Approve on 2017-08-14
Mir CI Bot continuous-integration Approve on 2017-08-14
Review via email: mp+328705@code.launchpad.net

Commit message

DefaultPersistentSurfaceStore: Wrap underlying store in a Mutex<>.

The surface store may be accessed from multiple threads (it's accessed from each
sessions SessionMediator, for example), so it should probably have some sort of
locking around its underlying datastore.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4220
https://mir-jenkins.ubuntu.com/job/mir-ci/3541/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4845
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5046
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5035
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5035
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5035
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4882/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4882
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4882/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

The motivation isn't quite as clear as commit message implies: the SessionMediator instances are all acting on the IPC thread. But it does make sense to make this threadsafe.

However, if we're going to introduce a smart RAII based lock (instead of sprinkling a few lock_guards in the access methods) I'd prefer the code using it to look nicer than:

    return (**(store.lock()))[id];

If you can make that, for example: "return (*store)[id]" I'll go along.

review: Needs Fixing
Alan Griffiths (alan-griffiths) wrote :

It is just possible this is a fix for lp:1629553

4221. By Chris Halse Rogers on 2017-08-11

mir::Mutex: Specialise for Mutex<>s that contain (smart-)pointers.

Chris Halse Rogers (raof) wrote :

It's not quite (*store)[id], but (**store)[id] is much nicer than (**(store.lock()))[id].

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4221
https://mir-jenkins.ubuntu.com/job/mir-ci/3551/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4863
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5066
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5055
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5055
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5055
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4900/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4900
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4900/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

OK, so there are a few issues and ideas:

1. the licence should (now) be GPL2 or GPL3

2. I'm not convinced by the user defined operators in MutexGuard they make it hard to use seamlessly with smart pointers.

3. Here's a sketch of an alternative approach:

=== modified file 'src/include/common/mir/mutex.h'
--- src/include/common/mir/mutex.h 2017-08-11 07:29:24 +0000
+++ src/include/common/mir/mutex.h 2017-08-11 16:32:12 +0000
@@ -62,6 +62,35 @@
     std::unique_lock<std::mutex> lock;
 };

+template<typename GuardedPtr>
+class MutexPtrGuard
+{
+public:
+
+ using GuardedPtrValue = typename std::pointer_traits<GuardedPtr>::element_type;
+
+ MutexPtrGuard(std::unique_lock<std::mutex>&& lock, GuardedPtr& value)
+ : value{value},
+ lock{std::move(lock)}
+ {
+ }
+ MutexPtrGuard(MutexPtrGuard&& from) = default;
+ ~MutexPtrGuard() noexcept(false)
+ {
+ if (lock.owns_lock())
+ {
+ lock.unlock();
+ }
+ }
+
+ GuardedPtrValue& operator*() const { return *value; }
+ GuardedPtrValue* operator->() const { return value.operator->(); }
+
+private:
+ GuardedPtr& value;
+ std::unique_lock<std::mutex> lock;
+};
+
 /**
  * A data-locking mutex
  *
@@ -106,11 +135,11 @@
      * While code has access to the MutexGuard it is guaranteed to have exclusive
      * access to the contained data.
      */
- MutexGuard<typename std::pointer_traits<Guarded>::element_type> operator*()
+ MutexPtrGuard<Guarded> operator->()
     {
- return MutexGuard<typename std::pointer_traits<Guarded>::element_type>{
+ return MutexPtrGuard<Guarded>{
             std::unique_lock<std::mutex>{mutex},
- *value
+ value
         };
     }

=== modified file 'src/server/shell/default_persistent_surface_store.cpp'
--- src/server/shell/default_persistent_surface_store.cpp 2017-08-11 07:29:24 +0000
+++ src/server/shell/default_persistent_surface_store.cpp 2017-08-11 16:09:29 +0000
@@ -82,14 +82,14 @@
 auto msh::DefaultPersistentSurfaceStore::id_for_surface(std::shared_ptr<scene::Surface> const& surface)
     -> Id
 {
- return (*store)->insert_or_retrieve(surface);
+ return store->insert_or_retrieve(surface);
 }

 std::shared_ptr<ms::Surface> msh::DefaultPersistentSurfaceStore::surface_for_id(Id const& id) const
 {
     try
     {
- return (**store)[id];
+ return store->operator[](id);
     }
     catch (std::out_of_range& err)
     {

(Yes I know that's not "(*store)[id]" either, but renaming SurfaceIdBimap::operator[] to a be normal member function wouldn't hurt.)

review: Needs Fixing
4222. By Chris Halse Rogers on 2017-08-14

DefaultPersistentSurfaceStore: Switch to std::mutex/lock_guard.

For this simple case it doesn't make much sense to use an smart-pointer-esque Mutex

Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:4222
https://mir-jenkins.ubuntu.com/job/mir-ci/3553/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/4865/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5078
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5067
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4902/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4902
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4902/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4902/console

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

review: Needs Fixing (continuous-integration)
Chris Halse Rogers (raof) wrote :

mir::Mutex might make an appearance on other merge proposals; particularly if I need to touch some of our classes with tens of members and multiple different mutexes protecting different subsets of them.

But for this one it is, indee, overkill.

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4222
https://mir-jenkins.ubuntu.com/job/mir-ci/3554/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4866
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5079
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5068
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5068
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5068
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4903/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4903
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4903/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/shell/default_persistent_surface_store.cpp'
2--- src/server/shell/default_persistent_surface_store.cpp 2017-07-28 17:00:43 +0000
3+++ src/server/shell/default_persistent_surface_store.cpp 2017-08-14 02:49:35 +0000
4@@ -82,6 +82,7 @@
5 auto msh::DefaultPersistentSurfaceStore::id_for_surface(std::shared_ptr<scene::Surface> const& surface)
6 -> Id
7 {
8+ std::lock_guard<std::mutex> lock{mutex};
9 return store->insert_or_retrieve(surface);
10 }
11
12@@ -89,6 +90,7 @@
13 {
14 try
15 {
16+ std::lock_guard<std::mutex> lock{mutex};
17 return (*store)[id];
18 }
19 catch (std::out_of_range& err)
20
21=== modified file 'src/server/shell/default_persistent_surface_store.h'
22--- src/server/shell/default_persistent_surface_store.h 2017-07-28 17:00:43 +0000
23+++ src/server/shell/default_persistent_surface_store.h 2017-08-14 02:49:35 +0000
24@@ -20,6 +20,7 @@
25 #define MIR_SHELL_DEFAULT_PERSISTENT_SURFACE_STORE_H_
26
27 #include "mir/shell/persistent_surface_store.h"
28+#include <mutex>
29
30 namespace mir
31 {
32@@ -36,6 +37,7 @@
33
34 private:
35 class SurfaceIdBimap;
36+ std::mutex mutable mutex;
37 std::unique_ptr<SurfaceIdBimap> const store;
38 };
39 }

Subscribers

People subscribed via source and target branches