Merge lp:~vanvugt/mir/fix-1194384 into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Extend the client API to support thread-safe data sharing between event
callbacks and the rest of the client. Also document the risks. (LP: #1194384)

No existing behaviour changes. Thread safety is optional, but recommended. :)

[lp:~vanvugt/mir/fix-1194384]

781. By Daniel van Vugt on 2013-06-26

Fix a possible race in:
TEST_F(DefaultDisplayServerTestFixture, client_event_handler_is_thread_safe)

[Daniel van Vugt (vanvugt)]

One can argue that this whole proposal is unnecessary, except for the docs improvements. Because client authors can use their own locking instead.

The rationale for the proposal then is: Without this, it is impossible to write a reliable, predictable client that handles events without also forcing the client to depend on some third-party locking primitives (pthread, C++11). That's really the issue. With all the features you may choose to include or exclude from an API, determinism is something you need to include.

Also, assuming that all toolkits are thread-aware is a bad assumption. I'm sure there are plenty of toolkits out there that don't have, or care about, threads. And Mir should support them too.

[Daniel van Vugt (vanvugt)]

One can also argue that a more reusable alternative would be to create a mir_mutex API for clients. That would work too...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:781
http://jenkins.qa.ubuntu.com/job/mir-ci/838/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1058
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/943
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/76
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/76/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Hmm, I forgot about non-event callbacks. They probably deserve consideration for a generic approach.

Work in progress again.

Unmerged revisions

781. By Daniel van Vugt on 2013-06-26

Fix a possible race in:
TEST_F(DefaultDisplayServerTestFixture, client_event_handler_is_thread_safe)

780. By Daniel van Vugt on 2013-06-26

Add acceptance test for {lock,unlock}_event_handler

779. By Daniel van Vugt on 2013-06-26

Clarify that set_event_handler is a synchronization point too.

778. By Daniel van Vugt on 2013-06-26

Prototype event handler locking

777. By Daniel van Vugt on 2013-06-26

Fix build failure due to differing exception specifiers. noexcept destructors
should be enforced in derived classes if the base uses them. (LP: #1194703). Fixes: https://bugs.launchpad.net/bugs/1194703.

Approved by Alexandros Frantzis, Thomas Voß, PS Jenkins bot.

776. By kevin gunn on 2013-06-26

this is to change the doc's fpr preinstalled binaries to reference using the system-compositor-testing ppa.

Approved by Robert Ancell, Kevin DuBois, PS Jenkins bot, Chris Halse Rogers.

775. By Robert Ancell on 2013-06-26

Update debian/copyright for 3rd_party files. Fixes: https://bugs.launchpad.net/bugs/1194073.

Approved by PS Jenkins bot.

774. By Alexandros Frantzis on 2013-06-25

graphics: Implement GLBufferPixels to extract buffer pixels using GL.

Approved by PS Jenkins bot.

773. By Daniel van Vugt on 2013-06-25

Don't pass a clang-only option to gcc. It will not understand and cause
build failure (LP: #1194385). Fixes: https://bugs.launchpad.net/bugs/1194385.

Approved by Alexandros Frantzis, PS Jenkins bot.

772. By Didier Roche-Tolomelli on 2013-06-25

Various packaging fixes:
- cleanup the package for daily release standard.
- multiarch the packaging.
- reorganize the packages, having demos and examples not shipped in usr/bin
  but rather in libexec.
- fix some dependency ordering.
- clean some olds transitions, lintian warnings, and cruft files.
- run tests on all archs and remove deprecated warnings.

Approved by PS Jenkins bot, Chris Halse Rogers, Kevin DuBois.

=== modified file 'examples/fingerpaint.c'
--- examples/fingerpaint.c	2013-06-13 12:12:58 +0000
+++ examples/fingerpaint.c	2013-06-26 11:58:26 +0000
@@ -253,7 +253,6 @@
     MirDisplayInfo dinfo;
     MirSurface *surf;
     MirGraphicsRegion canvas;
-    MirEventDelegate delegate = {&on_event, &canvas};
     int f;
 
     (void)argc;
@@ -290,7 +289,6 @@
     surf = mir_connection_create_surface_sync(conn, &parm);
     if (surf != NULL)
     {
-        mir_surface_set_event_handler(surf, &delegate);
     
         canvas.width = parm.width;
         canvas.height = parm.height;
@@ -300,12 +298,16 @@
 
         if (canvas.vaddr != NULL)
         {
+            MirEventDelegate delegate = {&on_event, &canvas};
+
             signal(SIGINT, shutdown);
             signal(SIGTERM, shutdown);
         
             clear_region(&canvas, &background);
             redraw(surf, &canvas);
         
+            mir_surface_set_event_handler(surf, &delegate);
+
             while (running)
             {
                 sleep(1);  /* Is there a better way yet? */
 
=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2013-06-20 09:43:30 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2013-06-26 11:58:26 +0000
@@ -136,12 +136,34 @@
 
 /**
  * Set the event handler to be called when events arrive for a surface.
+ *   \warning Event callbacks are called from a <b>different thread</b>, so if
+ *            you don't already have your own locking, you will need to use
+ *            mir_surface_lock_event_handler to protect any data shared between
+ *            event_handler and your other code.
  *   \param [in] surface        The surface
  *   \param [in] event_handler  The event handler to call
+ *   \post                      The old event handler (if any) has returned
  */
 void mir_surface_set_event_handler(MirSurface *surface,
                                    MirEventDelegate const *event_handler);
 
+/** 
+ * Acquire the mutex that surrounds event callbacks for a given surface. Use
+ * this to guarantee safe access to shared data you use inside the event
+ * handler.
+ *   \param [in] surface  The surface whose event handler to lock
+ *   \post                The event handler is not executing and will not be
+ *                        entered until you mir_surface_unlock_event_handler
+ */
+void mir_surface_lock_event_handler(MirSurface *surface);
+
+/** 
+ * Release the mutex that surrounds event callbacks for a given surface.
+ *   \pre                 You have called mir_surface_lock_event_handler
+ *   \param [in] surface  The surface whose event handler to unlock
+ */
+void mir_surface_unlock_event_handler(MirSurface *surface);
+
 /**
  * Get a window type that can be used for OpenGL ES 2.0 acceleration.
  *   \param [in] surface  The surface
 
=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-06-20 09:43:30 +0000
+++ src/client/mir_client_library.cpp	2013-06-26 11:58:26 +0000
@@ -178,6 +178,16 @@
     surface->set_event_handler(event_handler);
 }
 
+void mir_surface_lock_event_handler(MirSurface *surface)
+{
+    surface->lock_event_handler();
+}
+
+void mir_surface_unlock_event_handler(MirSurface *surface)
+{
+    surface->unlock_event_handler();
+}
+
 MirWaitHandle* mir_surface_release(
     MirSurface * surface,
     mir_surface_callback callback, void * context)
 
=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-06-20 09:43:30 +0000
+++ src/client/mir_surface.cpp	2013-06-26 11:58:26 +0000
@@ -43,7 +43,8 @@
     : server(server),
       connection(allocating_connection),
       buffer_depository(std::make_shared<mcl::ClientBufferDepository>(factory, mir::frontend::client_buffer_cache_size)),
-      input_platform(input_platform)
+      input_platform(input_platform),
+      event_delegate(nullptr)
 {
     mir::protobuf::SurfaceParameters message;
     message.set_surface_name(params.name ? params.name : std::string());
@@ -288,6 +289,14 @@
     return attrib_cache[at];
 }
 
+void MirSurface::event_callback(MirEvent const* event)
+{
+    lock_event_handler();
+    if (event_delegate)
+        event_delegate->callback(this, event, event_delegate->context);
+    unlock_event_handler();
+}
+
 void MirSurface::set_event_handler(MirEventDelegate const* delegate)
 {
     if (input_thread)
@@ -297,19 +306,32 @@
         input_thread = nullptr;
     }
 
-    if (delegate)
+    lock_event_handler();  // can only block if the user still owns it
+
+    event_delegate = delegate;  // can be NULL, to remove the handler
+
+    if (delegate && surface.fd_size() > 0)
     {
-        handle_event_callback = std::bind(delegate->callback, this,
-                                          std::placeholders::_1,
-                                          delegate->context);
+        auto handle_event_callback = std::bind(&MirSurface::event_callback,
+                                               this,
+                                               std::placeholders::_1);
 
-        if (surface.fd_size() > 0 && handle_event_callback)
-        {
-            input_thread = input_platform->create_input_thread(surface.fd(0),
+        input_thread = input_platform->create_input_thread(surface.fd(0),
                                                         handle_event_callback);
-            input_thread->start();
-        }
+        input_thread->start();
     }
+
+    unlock_event_handler();
+}
+
+void MirSurface::lock_event_handler()
+{
+    event_mutex.lock();
+}
+
+void MirSurface::unlock_event_handler()
+{
+    event_mutex.unlock();
 }
 
 void MirSurface::handle_event(MirEvent const& e)
@@ -321,8 +343,7 @@
             attrib_cache[a] = e.surface.value;
     }
 
-    if (handle_event_callback)
-        handle_event_callback(&e);
+    event_callback(&e);
 }
 
 MirPlatformType MirSurface::platform_type()
 
=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-06-20 09:43:30 +0000
+++ src/client/mir_surface.h	2013-06-26 11:58:26 +0000
@@ -31,7 +31,7 @@
 #include "client_platform.h"
 
 #include <memory>
-#include <functional>
+#include <mutex>
 
 namespace mir
 {
@@ -88,7 +88,10 @@
     MirWaitHandle* configure(MirSurfaceAttrib a, int value);
     int attrib(MirSurfaceAttrib a) const;
 
+    void event_callback(MirEvent const* event);
     void set_event_handler(MirEventDelegate const* delegate);
+    void lock_event_handler();
+    void unlock_event_handler();
     void handle_event(MirEvent const& e);
 
 private:
@@ -99,8 +102,6 @@
     void new_buffer(mir_surface_callback callback, void * context);
     mir::geometry::PixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf);
 
-    /* todo: race condition. protobuf does not guarantee that callbacks will be synchronized. potential
-             race in surface, last_buffer_id */
     mir::protobuf::DisplayServer::Stub & server;
     mir::protobuf::Surface surface;
     std::string error_message;
@@ -121,8 +122,11 @@
     // Cache of latest SurfaceSettings returned from the server
     int attrib_cache[mir_surface_attrib_arraysize_];
 
-    std::function<void(MirEvent const*)> handle_event_callback;
     std::shared_ptr<mir::input::receiver::InputReceiverThread> input_thread;
+
+    // event_mutex protects the execution of event_delgate->callback only.
+    std::mutex event_mutex;
+    const MirEventDelegate *event_delegate;
 };
 
 #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */
 
=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-06-06 16:44:49 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-06-26 11:58:26 +0000
@@ -27,6 +27,7 @@
 
 #include <gtest/gtest.h>
 #include <chrono>
+#include <thread>
 #include <cstring>
 
 namespace mf = mir::frontend;
@@ -345,6 +346,110 @@
     launch_client_process(client_config);
 }
 
+TEST_F(DefaultDisplayServerTestFixture, client_event_handler_is_thread_safe)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        static void event_callback(MirSurface* surface, MirEvent const* event,
+                                   void* ctx)
+        {
+            std::this_thread::sleep_for(std::chrono::seconds(1));
+            ClientConfig* self = static_cast<ClientConfig*>(ctx);
+            self->last_event = *event;
+            self->last_event_surface = surface;
+            self->prev_owner = self->owner;
+            self->owner = "event_callback";
+        }
+
+        void exec()
+        {
+            connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+            ASSERT_TRUE(connection != NULL);
+            ASSERT_TRUE(mir_connection_is_valid(connection));
+
+            MirSurfaceParameters const request_params =
+            {
+                __PRETTY_FUNCTION__,
+                640, 480,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware
+            };
+
+            memset(&last_event, 0, sizeof last_event);
+            last_event_surface = nullptr;
+            owner = "no one";
+            prev_owner = "nobody";
+
+            MirEventDelegate delegate{&event_callback, this};
+
+            surface =
+                mir_connection_create_surface_sync(connection, &request_params);
+            ASSERT_TRUE(surface != NULL);
+            ASSERT_TRUE(mir_surface_is_valid(surface));
+
+            mir_surface_set_event_handler(surface, &delegate);
+
+            int surface_id = mir_surface_get_id(surface);
+
+            mir_surface_lock_event_handler(surface);
+            MirWaitHandle *w = mir_surface_set_state(surface,
+                                               mir_surface_state_fullscreen);
+            std::this_thread::sleep_for(std::chrono::seconds(3));
+            // by now, event_callback is trying to start, but is blocked.
+            EXPECT_STREQ("no one", owner);
+            EXPECT_STREQ("nobody", prev_owner);
+            mir_surface_unlock_event_handler(surface);
+
+            mir_wait_for(w);
+            mir_surface_lock_event_handler(surface);
+            EXPECT_STREQ("event_callback", owner);
+            EXPECT_STREQ("no one", prev_owner);
+            prev_owner = owner;
+            owner = "exec";
+            mir_surface_unlock_event_handler(surface);
+
+            EXPECT_EQ(surface, last_event_surface);
+            EXPECT_EQ(mir_event_type_surface, last_event.type);
+            EXPECT_EQ(surface_id, last_event.surface.id);
+            EXPECT_EQ(mir_surface_attrib_state, last_event.surface.attrib);
+            EXPECT_EQ(mir_surface_state_fullscreen, last_event.surface.value);
+
+            memset(&last_event, 0, sizeof last_event);
+            last_event_surface = nullptr;
+
+            mir_wait_for(mir_surface_set_state(surface,
+                                           static_cast<MirSurfaceState>(777)));
+            EXPECT_EQ(0, last_event_surface);
+            EXPECT_EQ(0, last_event.type);
+            EXPECT_EQ(0, last_event.surface.id);
+            EXPECT_EQ(0, last_event.surface.attrib);
+            EXPECT_EQ(0, last_event.surface.value);
+
+            mir_surface_lock_event_handler(surface);
+            EXPECT_STREQ("exec", owner);
+            EXPECT_STREQ("event_callback", prev_owner);
+            mir_surface_unlock_event_handler(surface);
+
+            mir_wait_for(mir_surface_set_state(surface,
+                                               mir_surface_state_minimized));
+            mir_surface_set_event_handler(surface, nullptr);
+            // Event handler is dead. Locking no longer required.
+            EXPECT_STREQ("event_callback", owner);
+            EXPECT_STREQ("exec", prev_owner);
+            
+            mir_surface_release_sync(surface);
+            mir_connection_release(connection);
+        }
+
+        MirEvent last_event;
+        MirSurface* last_event_surface;
+        const char* prev_owner;
+        const char* owner;
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
 TEST_F(DefaultDisplayServerTestFixture, client_receives_surface_state_events)
 {
     struct ClientConfig : ClientConfigCommon