Merge lp:~vanvugt/mir/optional-callbacks into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Demonstrate how to make callbacks optional to the client API. Starting with
mir_connect() only. Callbacks are optionally attached to a MirWaitHandle.
This simplifies most client use-cases so you will see a lot of logic is
removed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:413
http://s-jenkins:8080/job/mir-ci/1085/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/mir-ci/./build=pbuilder,distribution=quantal,flavor=amd64/1085/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:415
http://s-jenkins:8080/job/mir-ci/1117/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/mir-ci/./build=pbuilder,distribution=quantal,flavor=amd64/1117/console

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

my initial reaction is that I like the idea of optional callbacks. Seeing as this is a change to the client api though, I'll mark as 'needs info' until we get more opinions on the matter in

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:418
http://jenkins.qa.ubuntu.com/job/mir-ci/20/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/20//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/20//rebuild/?

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

I think register_callback is a bit misleading - as you can have only one callback set_callback seems more appropriate.

Is both context and callback_owner required? This seems redundant.

I don't like the use of void pointers in the callbacks, that is a step backwards from having a typed callback in the client functions.

[Daniel van Vugt (vanvugt)]

Yeah I agree about set_callback. Not sure what I was thinking at the time. register_... sounds clumsy.

Yes context and owner are required to support the existing callbacks (client demos) without modification. But we could modify the clients to support single parameter callbacks.

Yeah I know there's a type safety issue with void* and have two proposals for fixing it:
  (1) Remove "owner" as mentioned above. Then "context" is opaque to Mir and is never cast; or
  (2) https://code.launchpad.net/~vanvugt/mir/simple-connect/+merge/147305

[Daniel van Vugt (vanvugt)]

Ok, "register" has been changed to "set".

I am willing to remove "owner" to improve type strength, but would want confirmation that people agree, before I invest the effort to modify all the clients and tests' callbacks.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:420
http://jenkins.qa.ubuntu.com/job/mir-ci/50/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/51//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/50//rebuild/?

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:420
http://jenkins.qa.ubuntu.com/job/mir-ci/52/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/53//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/52//rebuild/?

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:422
http://jenkins.qa.ubuntu.com/job/mir-ci/65/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/66//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/65//rebuild/?

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Proposed yet again. The currently open question is: Do people want me to convert the generic callbacks to single parameters in order to remove the type safety issue (hence remove "void *owner")?

[Daniel van Vugt (vanvugt)]

Actually, that might be a silly question. Removing the owner feature (self parameter of generic_callback) looks like a bad idea. I'd have to add significant logic (or significant casting assumptions) to get the result of an async call (e.g. MirConnection*). Not recommended.

[Daniel van Vugt (vanvugt)]

Wait, no. It is possible to do cleanly. I'll do it today.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:423
http://jenkins.qa.ubuntu.com/job/mir-ci/66/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/67//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/66//rebuild/?

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The discussions leading to the current design indicated that the principle use-case was that a callback would be supplied and the wait handle could (and usually would) be ignored.

The wait handles are a convenience for code that didn't manage its won synchronization - this proposal makes them a fundamental part of the callback based use-case.

There are definitely inelegances in the current API, but this complicates the principle use case.

review: Disapprove

[Daniel van Vugt (vanvugt)]

You will see the "principle use case" is actually not used once as yet. We only actually need callbacks right now in test cases to verify they work.

So the "principle use case" has come from hearsay requirements from people who actually aren't even using Mir yet. Once they do, they are very likely to discover the approach proposed here is preferable.

[Chris Halse Rogers (raof)]

This approach makes callback-based code slightly more annoying - it becomes

mir_callback_on(mir_connect(socket, name, &connection), callback, ctx);

rather than

mir_connect(socket, name, callback, ctx);

In return for this, it makes non-callback-based code - which is *everything* we've written so far - XMir, the Mesa EGL platform, all our demos - *significantly* less annoying.

It's not the API that I'd like, but it's a significant improvement on what we currently have.

As we don't have consensus on a better API, I'm willing to approve this on the basis that it'd be an acceptable compromise if we don't reach a different consensus.

review: Approve

[Chris Halse Rogers (raof)]

To the mailing list!

Unmerged revisions

423. By Daniel van Vugt on 2013-03-13

Simplified, and removed the type safety problem.

422. By Daniel van Vugt on 2013-03-13

Fix conflict (build failure) introduced by upstream changes.

421. By Daniel van Vugt on 2013-03-13

Merge latest lp:mir

420. By Daniel van Vugt on 2013-03-12

Clean up: "register_callback" --> "set_callback"

419. By Daniel van Vugt on 2013-03-12

Merge latest lp:mir

418. By Daniel van Vugt on 2013-03-08

Merge test_wait.cpp into test_wait_handle.cpp

417. By Daniel van Vugt on 2013-03-08

Fix conflicts and merge problems with upstream changes.

416. By Daniel van Vugt on 2013-03-08

Merge latest lp:mir

415. By Daniel van Vugt on 2013-03-01

Shrink the diff a little.

414. By Daniel van Vugt on 2013-03-01

Merge latest lp:mir and fix numerous conflicts with mir_toolkit changes.

=== modified file 'examples/demo_client.c'
--- examples/demo_client.c	2013-03-07 08:04:05 +0000
+++ examples/demo_client.c	2013-03-13 03:34:24 +0000
@@ -48,11 +48,6 @@
 /// needs to be accessible both to callbacks and to the control function.
 /// \snippet demo_client.c Callback_tag
 ///\internal [Callback_tag]
-// Callback to update MirDemoState on connection
-static void connection_callback(MirConnection *new_connection, void *context)
-{
-    ((MirDemoState*)context)->connection = new_connection;
-}
 
 // Callback to update MirDemoState on surface_create
 static void surface_create_callback(MirSurface *new_surface, void *context)
@@ -93,7 +88,7 @@
     /// \snippet demo_client.c connect_tag
     ///\internal [connect_tag]
     // Call mir_connect and wait for callback to complete.
-    mir_wait_for(mir_connect(server, __PRETTY_FUNCTION__, connection_callback, &mcd));
+    mir_wait_for(mir_connect(server, __PRETTY_FUNCTION__, &mcd.connection));
     puts("Connected");
     ///\internal [connect_tag]
 

=== modified file 'examples/demo_client_accelerated.cpp'
--- examples/demo_client_accelerated.cpp	2013-03-07 08:04:05 +0000
+++ examples/demo_client_accelerated.cpp	2013-03-13 03:34:24 +0000
@@ -32,12 +32,6 @@
 static MirConnection *connection = 0;
 static MirSurface *surface = 0;
 
-static void set_connection(MirConnection *new_connection, void * context)
-{
-    (void)context;
-    connection = new_connection;
-}
-
 static void surface_create_callback(MirSurface *new_surface, void *context)
 {
     (void)context;
@@ -76,7 +70,7 @@
 
     puts("Starting");
 
-    mir_wait_for(mir_connect(socket_file, __PRETTY_FUNCTION__, set_connection, 0));
+    mir_wait_for(mir_connect(socket_file, __PRETTY_FUNCTION__, &connection));
     puts("Connected");
 
     assert(connection != NULL);

=== modified file 'examples/demo_client_unaccelerated.c'
--- examples/demo_client_unaccelerated.c	2013-03-07 08:04:05 +0000
+++ examples/demo_client_unaccelerated.c	2013-03-13 03:34:24 +0000
@@ -30,12 +30,6 @@
 static MirConnection *connection = 0;
 static MirSurface *surface = 0;
 
-static void set_connection(MirConnection *new_connection, void * context)
-{
-    (void)context;
-    connection = new_connection;
-}
-
 static void surface_create_callback(MirSurface *new_surface, void *context)
 {
     (void)context;
@@ -136,7 +130,7 @@
 
     puts("Starting");
 
-    mir_wait_for(mir_connect(socket_file, __PRETTY_FUNCTION__, set_connection, 0));
+    mir_wait_for(mir_connect(socket_file, __PRETTY_FUNCTION__, &connection));
     puts("Connected");
 
     assert(connection != NULL);

=== modified file 'examples/eglapp.c'
--- examples/eglapp.c	2013-03-12 03:59:45 +0000
+++ examples/eglapp.c	2013-03-13 03:34:24 +0000
@@ -118,9 +118,7 @@
     EGLContext eglctx;
     EGLBoolean ok;
 
-    mir_wait_for(mir_connect(servername, appname,
-                             (mir_connected_callback)assign_result,
-                             &connection));
+    mir_wait_for(mir_connect(servername, appname, &connection));
     CHECK(mir_connection_is_valid(connection), "Can't get connection");
 
     mir_connection_get_display_info(connection, &dinfo);

=== modified file 'include/mir_toolkit/mir_client_library.h'
--- include/mir_toolkit/mir_client_library.h	2013-03-07 08:04:05 +0000
+++ include/mir_toolkit/mir_client_library.h	2013-03-13 03:34:24 +0000
@@ -41,12 +41,10 @@
 typedef struct MirWaitHandle MirWaitHandle;
 
 /**
- * Callback to be passed when issuing a mir_connect request.
- *   \param [in] connection          the new connection
- *   \param [in,out] client_context  context provided by client in calling
- *                                   mir_connect
+ * A generic callback that can be used with mir_callback_on().
+ *  \param [in] context  Context data for this callback.
  */
-typedef void (*mir_connected_callback)(MirConnection *connection, void *client_context);
+typedef void (*mir_generic_callback)(void *context);
 
 /**
  * Callback to be passed when calling:
@@ -147,18 +145,15 @@
  * Request a connection to the MIR server. The supplied callback is
  * called when the connection is established, or fails. The returned
  * wait handle remains valid until the connection has been released.
- *   \param [in] server              a name identifying the server
- *   \param [in] app_name            a name referring to the application
- *   \param [in] callback            callback function to be invoked when request
- *                                   completes
- *   \param [in,out] client_context  passed to the callback function
- *   \return                         a handle that can be passed to mir_wait_for
+ *   \param [in] server    a name identifying the server
+ *   \param [in] app_name  a name referring to the application
+ *   \param [out] result   where to store the connection handle on completion
+ *   \return               a handle that can be passed to mir_wait_for
  */
 MirWaitHandle *mir_connect(
     char const *server,
     char const *app_name,
-    mir_connected_callback callback,
-    void *client_context);
+    MirConnection **result);
 
 /**
  * Test for a valid connection
@@ -316,6 +311,17 @@
 void mir_wait_for(MirWaitHandle *wait_handle);
 
 /**
+ * Register a callback to be called when the specified wait handle is
+ * completed.
+ *   \param [in] wait     The wait handle to callback on.
+ *   \param [in] cb       The callback to call when 'wait' is signaled.
+ *   \param [in] context  Extra data to pass to the callback.
+ */
+void mir_callback_on(MirWaitHandle *wait,
+                     mir_generic_callback cb,
+                     void *context);
+
+/**
  * Return the id of the surface. (Only useful for debug output.)
  *   \param [in] surface  the surface
  *   \return              an internal id that identifies the surfaceS

=== modified file 'include/mir_toolkit/mir_client_library_lightdm.h'
--- include/mir_toolkit/mir_client_library_lightdm.h	2013-03-07 08:04:05 +0000
+++ include/mir_toolkit/mir_client_library_lightdm.h	2013-03-13 03:34:24 +0000
@@ -41,8 +41,7 @@
     char const *server,
     int lightdm_id,
     char const *app_name,
-    mir_connected_callback callback,
-    void *client_context);
+    MirConnection **result);
 
 
 /**

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-03-07 08:04:05 +0000
+++ src/client/mir_client_library.cpp	2013-03-13 03:34:24 +0000
@@ -44,7 +44,7 @@
 mir_toolkit::MirConnection error_connection;
 }
 
-mir_toolkit::MirWaitHandle* mir_toolkit::mir_connect(char const* socket_file, char const* name, mir_connected_callback callback, void * context)
+mir_toolkit::MirWaitHandle* mir_toolkit::mir_connect(char const* socket_file, char const* name, MirConnection **result)
 {
 
     try
@@ -57,12 +57,17 @@
             log,
             client_platform_factory);
 
-        return connection->connect(name, callback, context);
+        return connection->connect(name, result);
     }
     catch (std::exception const& x)
     {
         error_connection.set_error_message(x.what());
-        callback(&error_connection, context);
+
+        // Test cases expect a non-null error connection but why not null
+        // for simpler error handling?
+        if (result)
+            *result = &error_connection;
+
         return 0;
     }
 }
@@ -180,6 +185,14 @@
         wait_handle->wait_for_result();
 }
 
+void mir_toolkit::mir_callback_on(MirWaitHandle *wait,
+                     mir_generic_callback cb,
+                     void *context)
+{
+    if (wait)
+        wait->set_callback(cb, context);
+}
+
 mir_toolkit::MirEGLNativeWindowType mir_toolkit::mir_surface_get_egl_native_window(MirSurface *surface)
 {
     return surface->generate_native_window();
@@ -197,8 +210,7 @@
     char const *server,
     int lightdm_id,
     char const *app_name,
-    mir_connected_callback callback,
-    void *client_context)
+    MirConnection **result)
 try
 {
     auto log = std::make_shared<mcl::ConsoleLogger>();
@@ -209,12 +221,13 @@
         log,
         client_platform_factory);
 
-    return connection->connect(lightdm_id, app_name, callback, client_context);
+    return connection->connect(lightdm_id, app_name, result);
 }
 catch (std::exception const& x)
 {
     error_connection.set_error_message(x.what());
-    callback(&error_connection, client_context);
+    if (result)
+        *result = 0;
     return 0;
 }
 

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-03-07 08:04:05 +0000
+++ src/client/mir_connection.cpp	2013-03-13 03:34:24 +0000
@@ -126,7 +126,7 @@
     return new_wait_handle;
 }
 
-void mir_toolkit::MirConnection::connected(mir_connected_callback callback, void * context)
+void mir_toolkit::MirConnection::connected(MirConnection **result)
 {
     /*
      * We need to create the client platform after the connection has been
@@ -136,14 +136,15 @@
     platform = client_platform_factory->create_client_platform(this);
     native_display = platform->create_egl_native_display();
 
-    callback(this, context);
+    if (result)
+        *result = this;
+
     connect_wait_handle.result_received();
 }
 
 mir_toolkit::MirWaitHandle* mir_toolkit::MirConnection::connect(
     const char* app_name,
-    mir_connected_callback callback,
-    void * context)
+    MirConnection **result)
 {
     connect_parameters.set_application_name(app_name);
     server.connect(
@@ -151,15 +152,14 @@
         &connect_parameters,
         &connect_result,
         google::protobuf::NewCallback(
-            this, &MirConnection::connected, callback, context));
+            this, &MirConnection::connected, result));
     return &connect_wait_handle;
 }
 
 mir_toolkit::MirWaitHandle* mir_toolkit::MirConnection::connect(
     int lightdm_id,
     const char* app_name,
-    mir_connected_callback callback,
-    void * context)
+    MirConnection **result)
 {
     connect_parameters.set_application_name(app_name);
     connect_parameters.set_lightdm_id(lightdm_id);
@@ -168,7 +168,7 @@
         &connect_parameters,
         &connect_result,
         google::protobuf::NewCallback(
-            this, &MirConnection::connected, callback, context));
+            this, &MirConnection::connected, result));
     return &connect_wait_handle;
 }
 

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2013-03-07 08:04:05 +0000
+++ src/client/mir_connection.h	2013-03-13 03:34:24 +0000
@@ -71,14 +71,12 @@
 
     MirWaitHandle* connect(
         const char* app_name,
-        mir_connected_callback callback,
-        void * context);
+        MirConnection **result);
 
     MirWaitHandle* connect(
         int lightdm_id,
         const char* app_name,
-        mir_connected_callback callback,
-        void * context);
+        MirConnection **result);
 
     void select_focus_by_lightdm_id(int lightdm_id);
 
@@ -128,7 +126,7 @@
     struct SurfaceRelease;
 
     void done_disconnect();
-    void connected(mir_connected_callback callback, void * context);
+    void connected(MirConnection **result);
     void released(SurfaceRelease );
     void done_drm_auth_magic(mir_drm_auth_magic_callback callback, void* context);
 };

=== modified file 'src/client/mir_wait_handle.cpp'
--- src/client/mir_wait_handle.cpp	2013-03-08 07:20:43 +0000
+++ src/client/mir_wait_handle.cpp	2013-03-13 03:34:24 +0000
@@ -22,6 +22,9 @@
 mir_toolkit::MirWaitHandle::MirWaitHandle() :
     guard(),
     wait_condition(),
+    callback(nullptr),
+    callback_arg(nullptr),
+    called_back(false),
     expecting(0),
     received(0)
 {
@@ -42,6 +45,7 @@
 {
     std::unique_lock<std::mutex> lock(guard);
 
+    called_back = false;
     received++;
     wait_condition.notify_all();
 }
@@ -53,7 +57,27 @@
     while ((!expecting && !received) || (received < expecting))
         wait_condition.wait(lock);
 
+    if (!called_back && callback)
+    {
+        callback(callback_arg);
+        called_back = true;
+    }
+
     received = 0;
     expecting = 0;
 }
 
+void mir_toolkit::MirWaitHandle::set_callback(Callback cb, void *context)
+{
+    std::unique_lock<std::mutex> lock(guard);
+
+    callback = cb;
+    callback_arg = context;
+    called_back = false;
+    if (received && callback)
+    {
+        callback(callback_arg);
+        called_back = true;
+    }
+}
+

=== modified file 'src/client/mir_wait_handle.h'
--- src/client/mir_wait_handle.h	2013-03-08 07:20:43 +0000
+++ src/client/mir_wait_handle.h	2013-03-13 03:34:24 +0000
@@ -31,14 +31,20 @@
     MirWaitHandle();
     ~MirWaitHandle();
 
+    typedef void (*Callback)(void *context);
+
     void expect_result();
     void result_received();
     void wait_for_result();
+    void set_callback(Callback cb, void *context);
 
 private:
     std::mutex guard;
     std::condition_variable wait_condition;
 
+    Callback callback;
+    void *callback_arg;
+    bool called_back;
     int expecting;
     int received;
 };

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-03-07 08:04:05 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-03-13 03:34:24 +0000
@@ -49,14 +49,15 @@
     ClientConfigCommon()
         : connection(0)
         , surface(0),
-        buffers(0)
+        buffers(0),
+        called_back(false)
     {
     }
 
-    static void connection_callback(MirConnection * connection, void * context)
+    static void connection_callback(void *context)
     {
         ClientConfigCommon * config = reinterpret_cast<ClientConfigCommon *>(context);
-        config->connection = connection;
+        config->called_back = true;
     }
 
     static void create_surface_callback(MirSurface * surface, void * context)
@@ -100,6 +101,7 @@
     MirConnection* connection;
     MirSurface* surface;
     int buffers;
+    bool called_back;
 };
 }
 
@@ -109,10 +111,37 @@
     {
         void exec()
         {
-            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
-
-            ASSERT_TRUE(connection != NULL);
-            EXPECT_TRUE(mir_connection_is_valid(connection));
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
+
+            ASSERT_TRUE(connection != NULL);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            EXPECT_STREQ(mir_connection_get_error_message(connection), "");
+
+            mir_connection_release(connection);
+        }
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
+TEST_F(DefaultDisplayServerTestFixture, connect_calls_back)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        void exec()
+        {
+            called_back = false;
+
+            MirWaitHandle *wait = mir_connect(mir_test_socket,
+                                              __PRETTY_FUNCTION__,
+                                              &connection);
+            mir_callback_on(wait, connection_callback, this);
+            mir_wait_for(wait);
+
+            ASSERT_TRUE(connection != NULL);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            EXPECT_TRUE(called_back);
             EXPECT_STREQ(mir_connection_get_error_message(connection), "");
 
             mir_connection_release(connection);
@@ -129,7 +158,8 @@
         void exec()
         {
 
-            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             ASSERT_TRUE(connection != NULL);
             EXPECT_TRUE(mir_connection_is_valid(connection));
@@ -200,7 +230,8 @@
 
         void exec()
         {
-            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             ASSERT_TRUE(connection != NULL);
             EXPECT_TRUE(mir_connection_is_valid(connection));
@@ -252,7 +283,8 @@
         void exec()
         {
 
-            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             ASSERT_TRUE(connection != NULL);
             EXPECT_TRUE(mir_connection_is_valid(connection));
@@ -288,7 +320,8 @@
     {
         void exec()
         {
-            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
             ASSERT_TRUE(connection != NULL);
 
             MirPlatformPackage platform_package;
@@ -314,7 +347,8 @@
     {
         void exec()
         {
-            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
             ASSERT_TRUE(connection != NULL);
 
             MirDisplayInfo display_info;
@@ -338,7 +372,8 @@
     {
         void exec()
         {
-            mir_wait_for(mir_connect("garbage", __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect("garbage", __PRETTY_FUNCTION__,
+                                     &connection));
             ASSERT_TRUE(connection != NULL);
 
             char const* error = mir_connection_get_error_message(connection);
@@ -360,7 +395,8 @@
     {
         void exec()
         {
-            mir_wait_for(mir_connect("garbage", __PRETTY_FUNCTION__, connection_callback, this));
+            mir_wait_for(mir_connect("garbage", __PRETTY_FUNCTION__,
+                                     &connection));
 
             MirSurfaceParameters const request_params =
             {

=== modified file 'tests/acceptance-tests/test_focus_management_api.cpp'
--- tests/acceptance-tests/test_focus_management_api.cpp	2013-03-07 08:04:05 +0000
+++ tests/acceptance-tests/test_focus_management_api.cpp	2013-03-13 03:34:24 +0000
@@ -56,12 +56,6 @@
     {
     }
 
-    static void connection_callback(MirConnection* connection, void* context)
-    {
-        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
-        config->connection = connection;
-    }
-
     static void create_surface_callback(MirSurface* surface, void* context)
     {
         ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
@@ -200,8 +194,7 @@
                 mir_test_socket,
                 lightdm_id,
                 __PRETTY_FUNCTION__,
-                connection_callback,
-                this));
+                &connection));
 
             ASSERT_TRUE(connection != NULL);
 
@@ -229,8 +222,7 @@
             mir_wait_for(mir_connect(
                 mir_test_socket,
                 __PRETTY_FUNCTION__,
-                connection_callback,
-                this));
+                &connection));
 
             wait_for(focus_ready);
             mir_select_focus_by_lightdm_id(connection, lightdm_id);

=== modified file 'tests/acceptance-tests/test_surfaceloop.cpp'
--- tests/acceptance-tests/test_surfaceloop.cpp	2013-03-07 08:04:05 +0000
+++ tests/acceptance-tests/test_surfaceloop.cpp	2013-03-13 03:34:24 +0000
@@ -96,28 +96,6 @@
     {
     }
 
-    static void connection_callback(MirConnection * connection, void * context)
-    {
-        ClientConfigCommon * config = reinterpret_cast<ClientConfigCommon *>(context);
-        config->connected(connection);
-    }
-
-    void connected(MirConnection * new_connection)
-    {
-        std::unique_lock<std::mutex> lock(guard);
-        connection = new_connection;
-        wait_condition.notify_all();
-    }
-
-    void wait_for_connect()
-    {
-        std::unique_lock<std::mutex> lock(guard);
-        while (!connection)
-            wait_condition.wait(lock);
-    }
-
-    std::mutex guard;
-    std::condition_variable wait_condition;
     MirConnection * connection;
     static const int max_surface_count = 5;
     SurfaceSync ssync[max_surface_count];
@@ -160,9 +138,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             MirSurfaceParameters request_params =
             {
@@ -213,9 +190,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             MirSurfaceParameters request_params =
             {
@@ -257,9 +233,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             MirSurfaceParameters request_params =
             {

=== modified file 'tests/integration-tests/test_display_info.cpp'
--- tests/integration-tests/test_display_info.cpp	2013-03-07 08:04:05 +0000
+++ tests/integration-tests/test_display_info.cpp	2013-03-13 03:34:24 +0000
@@ -106,12 +106,6 @@
 
 };
 
-void connection_callback(MirConnection* connection, void* context)
-{
-    auto connection_ptr = static_cast<MirConnection**>(context);
-    *connection_ptr = connection;
-}
-
 }
 
 TEST_F(BespokeDisplayServerTestFixture, display_info_reaches_client)
@@ -139,7 +133,7 @@
         {
             MirConnection* connection{nullptr};
             mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
-                                     connection_callback, &connection));
+                                     &connection));
 
             MirDisplayInfo info;
 

=== modified file 'tests/integration-tests/test_drm_auth_magic.cpp'
--- tests/integration-tests/test_drm_auth_magic.cpp	2013-03-07 08:04:05 +0000
+++ tests/integration-tests/test_drm_auth_magic.cpp	2013-03-13 03:34:24 +0000
@@ -83,12 +83,6 @@
     MOCK_METHOD1(drm_auth_magic, void(unsigned int));
 };
 
-void connection_callback(MirConnection* connection, void* context)
-{
-    auto connection_ptr = static_cast<MirConnection**>(context);
-    *connection_ptr = connection;
-}
-
 void drm_auth_magic_callback(int status, void* client_context)
 {
     auto status_ptr = static_cast<int*>(client_context);
@@ -127,7 +121,7 @@
         {
             MirConnection* connection{nullptr};
             mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
-                                     connection_callback, &connection));
+                                     &connection));
 
             int const no_error{0};
             int status{67};
@@ -176,7 +170,7 @@
         {
             MirConnection* connection{nullptr};
             mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
-                                     connection_callback, &connection));
+                                     &connection));
 
             int status{67};
 

=== modified file 'tests/integration-tests/test_error_reporting.cpp'
--- tests/integration-tests/test_error_reporting.cpp	2013-03-07 08:04:05 +0000
+++ tests/integration-tests/test_error_reporting.cpp	2013-03-13 03:34:24 +0000
@@ -151,28 +151,6 @@
     {
     }
 
-    static void connection_callback(MirConnection * connection, void * context)
-    {
-        ClientConfigCommon * config = reinterpret_cast<ClientConfigCommon *>(context);
-        config->connected(connection);
-    }
-
-    void connected(MirConnection * new_connection)
-    {
-        std::unique_lock<std::mutex> lock(guard);
-        connection = new_connection;
-        wait_condition.notify_all();
-    }
-
-    void wait_for_connect()
-    {
-        std::unique_lock<std::mutex> lock(guard);
-        while (!connection)
-            wait_condition.wait(lock);
-    }
-
-    std::mutex guard;
-    std::condition_variable wait_condition;
     MirConnection * connection;
     static const int max_surface_count = 5;
     SurfaceSync ssync[max_surface_count];
@@ -255,9 +233,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             ASSERT_TRUE(connection != NULL);
             EXPECT_FALSE(mir_connection_is_valid(connection));

=== modified file 'tests/integration-tests/test_surfaceloop.cpp'
--- tests/integration-tests/test_surfaceloop.cpp	2013-03-07 08:04:05 +0000
+++ tests/integration-tests/test_surfaceloop.cpp	2013-03-13 03:34:24 +0000
@@ -183,28 +183,6 @@
     {
     }
 
-    static void connection_callback(MirConnection * connection, void * context)
-    {
-        ClientConfigCommon * config = reinterpret_cast<ClientConfigCommon *>(context);
-        config->connected(connection);
-    }
-
-    void connected(MirConnection * new_connection)
-    {
-        std::unique_lock<std::mutex> lock(guard);
-        connection = new_connection;
-        wait_condition.notify_all();
-    }
-
-    void wait_for_connect()
-    {
-        std::unique_lock<std::mutex> lock(guard);
-        while (!connection)
-            wait_condition.wait(lock);
-    }
-
-    std::mutex guard;
-    std::condition_variable wait_condition;
     MirConnection * connection;
     static const int max_surface_count = 5;
     SurfaceSync ssync[max_surface_count];
@@ -270,9 +248,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             ASSERT_TRUE(connection != NULL);
             EXPECT_TRUE(mir_connection_is_valid(connection));
@@ -373,9 +350,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             ASSERT_TRUE(connection != NULL);
             EXPECT_TRUE(mir_connection_is_valid(connection));
@@ -512,9 +488,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             MirSurfaceParameters const request_params =
             {
@@ -561,9 +536,8 @@
     {
         void exec()
         {
-            mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this);
-
-            wait_for_connect();
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__,
+                                     &connection));
 
             MirSurfaceParameters const request_params =
             {

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-07 08:04:05 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-13 03:34:24 +0000
@@ -201,10 +201,6 @@
 
 namespace mt = mir::test;
 
-void connected_callback(MirConnection* /*connection*/, void * /*client_context*/)
-{
-}
-
 struct CallBack
 {
     void msg() {}
@@ -233,7 +229,7 @@
         channel = mcl::make_rpc_channel("./test_socket_surface", logger);
         connection = std::make_shared<MirConnection>(channel, logger, platform_factory);
         MirWaitHandle* wait_handle = connection->connect("MirClientSurfaceTest",
-                                                         connected_callback, 0);
+                                                         nullptr);
         wait_handle->wait_for_result();
         client_comm_channel = std::make_shared<mir::protobuf::DisplayServer::Stub>(channel.get());
     }

=== modified file 'tests/unit-tests/client/test_mir_connection.cpp'
--- tests/unit-tests/client/test_mir_connection.cpp	2013-01-02 15:57:13 +0000
+++ tests/unit-tests/client/test_mir_connection.cpp	2013-03-13 03:34:24 +0000
@@ -98,10 +98,6 @@
     std::shared_ptr<mcl::ClientPlatform> platform;
 };
 
-void connected_callback(MirConnection* /*connection*/, void * /*client_context*/)
-{
-}
-
 void drm_auth_magic_callback(int status, void* client_context)
 {
     auto status_ptr = static_cast<int*>(client_context);
@@ -144,7 +140,7 @@
         .WillOnce(Return(native_display));
 
     MirWaitHandle* wait_handle = connection->connect("MirClientSurfaceTest",
-                                                     connected_callback, 0);
+                                                     nullptr);
     wait_handle->wait_for_result();
 
     EGLNativeDisplayType connection_native_display = connection->egl_native_display();
@@ -167,7 +163,7 @@
         .Times(1);
 
     MirWaitHandle* wait_handle = connection->connect("MirClientSurfaceTest",
-                                                     connected_callback, 0);
+                                                     nullptr);
     wait_handle->wait_for_result();
 
     int const no_error{0};
@@ -211,7 +207,7 @@
         .WillOnce(Invoke(fill_display_info));
 
     MirWaitHandle* wait_handle = connection->connect("MirClientSurfaceTest",
-                                                     connected_callback, 0);
+                                                     nullptr);
     wait_handle->wait_for_result();
 
     MirDisplayInfo info;
@@ -236,7 +232,7 @@
         .WillOnce(Invoke(fill_display_info_100));
 
     MirWaitHandle* wait_handle = connection->connect("MirConnectionTest",
-                                                     connected_callback, 0);
+                                                     nullptr);
     wait_handle->wait_for_result();
 
     MirDisplayInfo info;

=== modified file 'tests/unit-tests/client/test_wait_handle.cpp'
--- tests/unit-tests/client/test_wait_handle.cpp	2013-03-07 07:19:39 +0000
+++ tests/unit-tests/client/test_wait_handle.cpp	2013-03-13 03:34:24 +0000
@@ -112,3 +112,78 @@
     }
     t.join();
 }
+
+TEST(WaitHandle, no_callback)
+{
+    MirWaitHandle w;
+
+    w.result_received();
+    EXPECT_NO_FATAL_FAILURE(w.wait_for_result());
+}
+
+namespace
+{
+    void *cb_context = nullptr;
+
+    void callback(void *context)
+    {
+        cb_context = context;
+    }
+}
+
+TEST(WaitHandle, callback_before_signal)
+{
+    MirWaitHandle w;
+    int beta, delta;
+
+    cb_context = 0;
+    w.set_callback(callback, &beta);
+    w.result_received();
+    w.wait_for_result();
+    EXPECT_EQ(cb_context, &beta);
+
+    w.result_received();
+    w.wait_for_result();
+    EXPECT_EQ(cb_context, &beta);
+
+    cb_context = 0;
+    w.set_callback(callback, &delta);
+    w.result_received();
+    w.wait_for_result();
+    EXPECT_EQ(cb_context, &delta);
+
+    w.set_callback(nullptr, nullptr);
+    EXPECT_NO_FATAL_FAILURE({
+        w.result_received();
+        w.wait_for_result();
+    });
+}
+
+TEST(WaitHandle, callback_after_signal)
+{
+    MirWaitHandle w;
+    int beta, delta;
+
+    cb_context = 0;
+    w.result_received();
+    w.set_callback(callback, &beta);
+    w.wait_for_result();
+    EXPECT_EQ(cb_context, &beta);
+
+    w.result_received();
+    w.wait_for_result();
+    EXPECT_EQ(cb_context, &beta);
+
+    cb_context = 0;
+
+    w.result_received();
+    w.set_callback(callback, &delta);
+    w.wait_for_result();
+    EXPECT_EQ(cb_context, &delta);
+
+    w.set_callback(nullptr, nullptr);
+    EXPECT_NO_FATAL_FAILURE({
+        w.result_received();
+        w.wait_for_result();
+    });
+}